Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: stock market service and info widget #3617

Merged
merged 8 commits into from
Jul 2, 2024

Conversation

eldyl
Copy link
Contributor

@eldyl eldyl commented Jun 9, 2024

Proposed change

Hello!

I have been working on an implementation of a stock market widget.

Relevant Feature Request - #297

widget_stocks_screenshot

homepage_widget_stocks_demo

Type of change

  • New service widget
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Documentation only
  • Other (please explain) Information widget

Checklist:

  • If applicable, I have added corresponding documentation changes.
  • If applicable, I have reviewed the feature and / or service widget guidelines.
  • I have checked that all code style checks pass using pre-commit hooks and linting checks.
  • If applicable, I have tested my code for new features & regressions on both mobile & desktop devices, using the latest version of major browsers.

Notes

  • I felt like this widget fit best as an info widget, but I am happy to change things if the category is not appropriate.
  • My goal was to show basic information in a simple way.
  • Uses Finnhub.io's API. You can sign up for a free API key.
  • You can click on the widget to switch between the current price of a stock and the change in price of the stock since open.
  • I created the widget as the "Stocks" widget rather than "Finnhub". The intention was to allow different APIs as providers. It should be relatively straightforward to implement more APIs.

Desktop

homepage_widget_stocks_desktop

Mobile

homepage_widget_stocks_mobile

Copy link
Collaborator

@shamoon shamoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is great overall and I think people will be happy with it. Especially like that its extensible.

A couple small things before I double-check the code is that as-is this will allow duplicates in the list (thats prob OK, people can do crazy things, then again it'd be easy enough to solve) but Im also wondering if the limit of 30 (which I know comes from the API) is too much. It just starts to get too large. Maybe thats OK but my gut tells me we should trim it down. What do you think?

Screenshot from 2024-06-09 18-56-25

@eldyl
Copy link
Contributor Author

eldyl commented Jun 10, 2024

I appreciate your feedback!

as-is this will allow duplicates in the list

I will take care of this.

Im also wondering if the limit of 30 (which I know comes from the API) is too much. It just starts to get too large. Maybe thats OK but my gut tells me we should trim it down. What do you think?

Example with 6 Items
6

Example with 8 Items
8

Agreed. A max of 6 kind of feels like the sweet spot. With a known max, I would also like to handle some viewport sizes a little differently.

If it works, I can have these updates included tomorrow.

If there is anything else that looks like it could be improved, I am happy to make the changes.

@shamoon
Copy link
Collaborator

shamoon commented Jun 10, 2024

Yea no problem, sounds good on all accounts. I’ll take a look at the code more closely after you push the changes but overall seems like pretty a clean PR thanks 👍

@brhahlen
Copy link

Yay! Love this!
My only question would be if it would be possible to make a service widget for it? Would fit better with the CMC widget as welll :)
Thanks for the hard work!

@eldyl
Copy link
Contributor Author

eldyl commented Jun 10, 2024

Yay! Love this! My only question would be if it would be possible to make a service widget for it? Would fit better with the CMC widget as welll :) Thanks for the hard work!

I could do both.

The component looks like it is pretty easily adaptable:
https://github.com/gethomepage/homepage/blob/main/src/widgets/coinmarketcap/component.jsx

@shamoon What are your thoughts? Would it be better to pick one? Or would it be reasonable to have both component types as potential options?

@shamoon
Copy link
Collaborator

shamoon commented Jun 10, 2024

It's not clear from the FR if people want a service widget or info, so it's of course debatable. But given this is closest to the CoinMarketCap widget (which btw is better suited to the list format) perhaps that is the better way to go, sorry for the changeup.

@eldyl
Copy link
Contributor Author

eldyl commented Jun 10, 2024

Happy to make the change. I agree that the format of the CoinMarketCap widget does work better overall. I can get started later today.

@shamoon shamoon marked this pull request as draft June 11, 2024 19:52
@benphelps
Copy link
Member

I think both should be options.

On the layout of the info widget, I think keeping the font size the same, and lowering the opacity of the price, would be more inline with the Homepage design direction. This would also allow it to use slightly less vertical space.

@eldyl
Copy link
Contributor Author

eldyl commented Jun 14, 2024

I think both should be options.

For my personal use, I do prefer having a few items in the header in most instances.

On the layout of the info widget, I think keeping the font size the same, and lowering the opacity of the price, would be more inline with the Homepage design direction. This would also allow it to use slightly less vertical space.

I will look into these changes. Thanks for the tips.


Apologies for the delay. I have been busy with work :)

I was testing some other APIs to see if it makes sense to include them in the widget, but I think, at least for now, Finnhub seems like a better option with their free tier and limits.

I also have tested getting data from https://finance.yahoo.com/quotes/${comma_separated_list_of_tickers}. This would work without an API key, but will require some more processing. I have been testing headers to reduce the size of the received payload.

I suggested this could be a Stocks, widget, and I'd like to make that work, but I would like to provide proof of concept with multiple APIs. Otherwise, it might make sense for for the Widget to be a Finnhub widget. I also want to make sure it won't be difficult to maintain multiple APIs. That being said, if changes in the future are accepted, I am happy to help maintain them.

@shamoon
Copy link
Collaborator

shamoon commented Jun 14, 2024

I would just focus on this version, especially given it will now support two versions of the widget let’s keep the PR focused ie MVP. I’m confident this can be adapted for other APIs if there is need

@eldyl
Copy link
Contributor Author

eldyl commented Jun 17, 2024

I think both should be options.

On the layout of the info widget, I think keeping the font size the same, and lowering the opacity of the price, would be more inline with the Homepage design direction. This would also allow it to use slightly less vertical space.

Maybe something closer to this?

homepage_stock_info_preview_1

homepage_preview_diff_w_color

@github-actions github-actions bot added the stale label Jun 26, 2024
@eldyl eldyl force-pushed the feature/widget_stocks branch 4 times, most recently from 2008e5f to 878d62b Compare July 1, 2024 05:20
@eldyl
Copy link
Contributor Author

eldyl commented Jul 1, 2024

I will write with some more information in the morning regarding some of the choices I made, but this should be ready for review.

mobile
desktop

@eldyl eldyl marked this pull request as ready for review July 1, 2024 05:30
docs/widgets/info/stocks.md Outdated Show resolved Hide resolved
@shamoon shamoon changed the title Feature: add stock market data widget Feature: stock market service and info widget Jul 1, 2024
@shamoon
Copy link
Collaborator

shamoon commented Jul 1, 2024

Looking good. Small thing, I wouldn’t bother with displaying (or translating) the errors or displaying things like has duplicates, it’s basically a config error so just throw an error with details in the log and / or truncate the list / remove the duplicates.

Copy link
Collaborator

@shamoon shamoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I havent tested yet, just some thoughts looking at the code. Thanks

src/components/widgets/stocks/stocks.jsx Outdated Show resolved Hide resolved
src/components/widgets/stocks/stocks.jsx Outdated Show resolved Hide resolved
src/components/widgets/stocks/stocks.jsx Outdated Show resolved Hide resolved
src/components/widgets/stocks/stocks.jsx Outdated Show resolved Hide resolved
src/pages/api/widgets/stocks.js Outdated Show resolved Hide resolved
src/pages/api/widgets/stocks.js Outdated Show resolved Hide resolved
src/widgets/stocks/component.jsx Outdated Show resolved Hide resolved
src/widgets/stocks/component.jsx Outdated Show resolved Hide resolved
@eldyl
Copy link
Contributor Author

eldyl commented Jul 1, 2024

Reviewing now. Thank you for these great suggestions. I will try and get them all added today.

ran pre-commit hook

improved translations and error handling

include service widget

fix: update docs image

use original spacing in yaml file

fix: typo in docs

use Error component

use regular error messaging here

fix: make use of standard config error

fix: map through data in jsx

fix: simplify parsing/error logic
@eldyl
Copy link
Contributor Author

eldyl commented Jul 1, 2024

I havent tested yet, just some thoughts looking at the code. Thanks

Mentioned issues should be resolved: https://github.com/gethomepage/homepage/compare/878d62b1f832f17bb57d18ac3b00ad70dec0c2df..ae3df9eeeb045f256bce33751d1d504ca96cf6cd

I really appreciate the feedback :)

@shamoon shamoon enabled auto-merge (squash) July 2, 2024 00:13
Copy link
Collaborator

@shamoon shamoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, I think people will dig this. some minor tweaks but nothing major
Screenshot 2024-07-01 at 5 06 49 PM
Screenshot 2024-07-01 at 5 06 57 PM

@shamoon shamoon merged commit 231e240 into gethomepage:main Jul 2, 2024
2 checks passed
@eldyl
Copy link
Contributor Author

eldyl commented Jul 2, 2024

Great work, I think people will dig this. some minor tweaks but nothing major Screenshot 2024-07-01 at 5 06 49 PM Screenshot 2024-07-01 at 5 06 57 PM

Thanks again for your feedback throughout. I checked your changes, and I made some notes for myself as well.

Thanks!

@vednolacni
Copy link

Is it possible to add crypto?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants