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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support other currencies #192

Merged
merged 9 commits into from
Oct 4, 2022
Merged

Conversation

ekzyis
Copy link
Member

@ekzyis ekzyis commented Sep 12, 2022

Hey! Thanks for your website, I have really fallen in love with it and visit it every day!

But I missed a user setting to choose the preferred currency, so I added it; open source ftw! 馃槃 (closes #103)

I have only added EUR, but other currencies which coinbase supports are easy to add. Just add it together with the symbol to CURRENCY_SYMBOLS in components/price.js.

At first, I wanted to use a dropdown to choose the currency in the settings, but I didn't found a dropdown I could reuse, so I just went with a text input first.

Please tell me if you like it and if I should adapt anything 馃憤

@ekzyis
Copy link
Member Author

ekzyis commented Sep 13, 2022

Some impressions:

2022-09-13-021754_1920x1080_scrot

2022-09-13-021810_1920x1080_scrot

2022-09-13-021841_1920x1080_scrot

@huumn
Copy link
Member

huumn commented Sep 13, 2022

Overall looks pretty good. I'm pretty swamped with stuff but I'll try to take a closer look soon.

I agree a dropdown would be a much better UX.

@ekzyis
Copy link
Member Author

ekzyis commented Sep 18, 2022

I implemented it as a dropdown now. Also added currencies suggested by a user here: https://stacker.news/items/69184

Also fixed some errors if the user is not logged in yet and thus fiatCurrency does not exist on the me object in 22e07a4

2022-09-18-035437_1920x1080_scrot

@huumn
Copy link
Member

huumn commented Sep 19, 2022

Nice job! I'll try to merge this soon

@ekzyis
Copy link
Member Author

ekzyis commented Oct 3, 2022

I see there are conflicts and the dropdown has the same issue as described in #200.

Will resolve conflicts and check dropdown scrollbars soon.

@huumn
Copy link
Member

huumn commented Oct 3, 2022

My bad. Merged out of order.

I plan to merge this PR and the other remaining tomorrow. I can fix myself if you don't have time.

@ekzyis
Copy link
Member Author

ekzyis commented Oct 3, 2022

My bad. Merged out of order.

I plan to merge this PR and the other remaining tomorrow. I can fix myself if you don't have time.

Haha, no worries. I actually like resolving conflicts. I used the Github GUI now. It is actually decent for small conflicts like these.

I didn't check #200 however. Not sure if that needs a separate fix here or will be automatically fixed because it uses the same CSS (most likely)

@huumn huumn merged commit 0ff9bbc into stackernews:master Oct 4, 2022
@ekzyis ekzyis deleted the 103-add-other-currencies branch October 15, 2022 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add other currencies for displaying bitcoin price
2 participants