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

Widget: Bitcoin #1999

Closed
wants to merge 9 commits into from
Closed

Widget: Bitcoin #1999

wants to merge 9 commits into from

Conversation

cacpmw
Copy link

@cacpmw cacpmw commented Apr 5, 2024

Category

Widget

Overview

A very simple bitcoin widget to fetch current price in USD and EUR plus the current recommended fees per transaction. It uses the free mempool api
I intend to improve this widget over time but anyone is welcome to make it more useful

Screenshot (if applicable)

Screenshot from 2024-04-05 17-52-27

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hi 👋. Thank you for making your first contribution to Homarr. Please ensure that you've completed all the points in the TODO checklist. We'll review your changes shortly.

Copy link
Owner

@ajnart ajnart left a comment

Choose a reason for hiding this comment

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

Hello! Thanks for the PR, I just saw your LinkedIn post! Congrats on your contribution. After reviewing it quickly I can say that I do not approve of this Widget simply because it is not common enough, I'm sure a lot of people are interested in crypto but adding a widget only to display the price of bitcoin is not good enough. We already support iframe and the website https://coinlib.io/widgets?w_single_coin_id=859&w_single_pref_coin_id=1505&w_single_width=250&w_all_theme=Light#w_single can basically create this widget with the selection of the coin, the currency, …

However, you could most likely find a read API endpoint (without a library) that gives out that info and more (like the price for the last X days) and add options for example : selected coin (XRP, ADA, ETH, BTC…) , timeframe (24h, 7d, 30d, 1y) and display a cool graph using our included graph library. I know this is a lot of work but if you need help I can help you. Feel free to reach on discord about this.

Do you think my review is too harsh ? Please don't get frustrated by my refusal, I am very happy to welcome new contributors. It's just that we need to have high quality widgets. Let's work together on making a high quality "crypto" widget and not only a bitcoin one ;)

data/configs/default.json Outdated Show resolved Hide resolved
public/locales/en/modules/bitcoin.json Outdated Show resolved Hide resolved
src/widgets/bitcoin/BitcoinWidgetTile.tsx Outdated Show resolved Hide resolved
@ajnart
Copy link
Owner

ajnart commented Apr 5, 2024

Also please note that your commits are unverified and not authored by you in any way, there is an issue with your git config. The committer is called "Your name"

@cacpmw
Copy link
Author

cacpmw commented Apr 5, 2024

Hello! Thanks for the PR, I just saw your LinkedIn post! Congrats on your contribution. After reviewing it quickly I can say that I do not approve of this Widget simply because it is not common enough, I'm sure a lot of people are interested in crypto but adding a widget only to display the price of bitcoin is not good enough. We already support iframe and the website https://coinlib.io/widgets?w_single_coin_id=859&w_single_pref_coin_id=1505&w_single_width=250&w_all_theme=Light#w_single can basically create this widget with the selection of the coin, the currency, …

However, you could most likely find a read API endpoint (without a library) that gives out that info and more (like the price for the last X days) and add options for example : selected coin (XRP, ADA, ETH, BTC…) , timeframe (24h, 7d, 30d, 1y) and display a cool graph using our included graph library. I know this is a lot of work but if you need help I can help you. Feel free to reach on discord about this.

Do you think my review is too harsh ? Please don't get frustrated by my refusal, I am very happy to welcome new contributors. It's just that we need to have high quality widgets. Let's work together on making a high quality "crypto" widget and not only a bitcoin one ;)

I made this widget out of my own need at this moment. I know it could be more elaborated and show fancy graphs and all that stuff. It was my intention to increment it over time but for now the most important thing is having BTC price at my fingertips without having to leave my dashboard.

As of add other coins I have no problem with that I just think they are irrelevant but as I said I welcome anyone wiling to make this widget more useful. I built it out of my own needs.

a Crypto.com study found that more than 219 million individual Bitcoin holders were recorded in late 2022

Anyway, your suggestions are indeed good ones and I will take them but I am extremely bad at styling frontend and I couldn't find any docs regarding development standards. This project is huge and it was quite hard to find myself in it, like I was very confused where to store translation file for instance, whether I could write the api my own way or should I follow some pattern etc. I used the weatherWidget as my reference.

I could totally build all endpoints to all your request with my hands tied on my back but I suck and styling things on frontend and also I am always unsure on the coding standards. I am going on sorta trial and error basis here.

I don't consider your review harsh, it is direct to the point which is what I expect. I gladly accept your help, if you can take charge of the frontend that would be amazing and make things much more faster.

As of my git configs you are right, I had forgotten to set it up. I will if there is a way to fix the commits

@cacpmw cacpmw marked this pull request as draft April 5, 2024 22:54
@ajnart
Copy link
Owner

ajnart commented Apr 5, 2024

You should try to implement things on your own. Try to understand how the weather widget works, remove the new dependency and use a public API. Then look again at the weather widget to understand options and how to use them. Then use these options in your API calls (locally using tanstack query or in the router like you did)
And then you should have a functioning crypto widget ;)

We simply cannot merge the "bitcoin" widget as it adds no value (because of the already existing iframe widget) the added cost in dependencies would make the app use more ram/storage as well.

@cacpmw
Copy link
Author

cacpmw commented Apr 9, 2024

@ajnart can you help me find out what I am missing.

I found a general cryptocurrency api (coingecko) and refactor the whole damn thing in server/api/routers/cryptocurrencies/.

For some reason I cant get data to the frontend, something is not working properly on my router I cant seem to find out what. But this what I get in the frontend:

image

Now here is the thing, if throw and fucking console log in the in api code I get the data correctly and if I simply throw the url on the browser I get data correctly... I know the urls are correct

@ajnart
Copy link
Owner

ajnart commented Apr 10, 2024

@ajnart can you help me find out what I am missing.

I found a general cryptocurrency api (coingecko) and refactor the whole damn thing in server/api/routers/cryptocurrencies/.

For some reason I cant get data to the frontend, something is not working properly on my router I cant seem to find out what. But this what I get in the frontend:

image

Now here is the thing, if throw and fucking console log in the in api code I get the data correctly and if I simply throw the url on the browser I get data correctly... I know the urls are correct

It seems like you are calling the procedure without arguments, (symbol, name, links)

Ideally you just have a symbol, timeframe option and the procedure will return you the price/fluctuation of that asset in the correct timeframe. You just need to make it so that the option is then properly interpolated onto the URL that is used to get the data from the api.

But your editor should say that you are calling the procedure without correct arguments so I'm unsure 🫤 you can push your code and we can see where you have errors

@ajnart
Copy link
Owner

ajnart commented Apr 10, 2024

After glancing at the code, I'm not sure why you have another call for "get initial data", I think you can just use the basic call to populate your data

@cacpmw
Copy link
Author

cacpmw commented Apr 10, 2024

@ajnart can you help me find out what I am missing.
I found a general cryptocurrency api (coingecko) and refactor the whole damn thing in server/api/routers/cryptocurrencies/.
For some reason I cant get data to the frontend, something is not working properly on my router I cant seem to find out what. But this what I get in the frontend:
image
Now here is the thing, if throw and fucking console log in the in api code I get the data correctly and if I simply throw the url on the browser I get data correctly... I know the urls are correct

It seems like you are calling the procedure without arguments, (symbol, name, links)

Ideally you just have a symbol, timeframe option and the procedure will return you the price/fluctuation of that asset in the correct timeframe. You just need to make it so that the option is then properly interpolated onto the URL that is used to get the data from the api.

But your editor should say that you are calling the procedure without correct arguments so I'm unsure 🫤 you can push your code and we can see where you have errors

Those are not arguments, those are the zod schema I created for the response. It seems like the procedure is not awaiting response from the api. Also I defined default values for each procedure in case no input is passed

@cacpmw
Copy link
Author

cacpmw commented Apr 10, 2024

After glancing at the code, I'm not sure why you have another call for "get initial data", I think you can just use the basic call to populate your data

The reason a made getInitialData is simply to have both calls inside only one since they always happen together in order to update the widget

@ajnart
Copy link
Owner

ajnart commented Apr 10, 2024

After glancing at the code, I'm not sure why you have another call for "get initial data", I think you can just use the basic call to populate your data

The reason a made getInitialData is simply to have both calls inside only one since they always happen together in order to update the widget

Why not just have one inside of one procedure ?

@cacpmw
Copy link
Author

cacpmw commented Apr 10, 2024

Isnt that what this is?

  getInitialData: publicProcedure
    .output(cryptoWidgetDataSchema)
    .query(async () => getInitialData()),

@cacpmw
Copy link
Author

cacpmw commented Apr 10, 2024

After glancing at the code, I'm not sure why you have another call for "get initial data", I think you can just use the basic call to populate your data

The reason a made getInitialData is simply to have both calls inside only one since they always happen together in order to update the widget

Why not just have one inside of one procedure ?

if I make this call:
api.cryptoCurrencies.getCoinGeckoCryptoData.useQuery()
it works

apparently the problem is on this call api.cryptoCurrencies.getCoinGeckoCryptoMarketChart.useQuery()

I will try to find my mistake

@cacpmw
Copy link
Author

cacpmw commented Apr 10, 2024

After the request is made using fetch I get this:
image
Note that the url is correct formed. Even if I pass empty object to useQuery() in the frontend the url is well formed by the default values.

Copy link

github-actions bot commented Jun 9, 2024

Hello 👋, this PR has gone stale. Please reply to mark it as active.

@github-actions github-actions bot added the Stale label Jun 9, 2024
@ajnart ajnart closed this Aug 8, 2024
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.

2 participants