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

Show sponsor goal progress bar #347

Merged
merged 27 commits into from
Dec 9, 2024
Merged

Show sponsor goal progress bar #347

merged 27 commits into from
Dec 9, 2024

Conversation

alainm23
Copy link
Contributor

@alainm23 alainm23 commented Dec 4, 2024

Fixed #329

image

@alainm23 alainm23 marked this pull request as ready for review December 4, 2024 22:43
@alainm23

This comment has been minimized.

@alainm23 alainm23 requested a review from danirabbit December 6, 2024 12:23
@danirabbit
Copy link
Member

@tintou any thoughts on how a github token should be stored?

Copy link
Member

@danirabbit danirabbit left a comment

Choose a reason for hiding this comment

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

All the UI parts of this look good to me! Localization, dark mode, etc all seems good

I think I'd like another opinion about if there's a better way to store a token 😅 But otherwise I'm happy with this 🎉

@alainm23
Copy link
Contributor Author

alainm23 commented Dec 6, 2024

@danirabbit we have a problem with the type of currency, in your case that you live in the USA, the type of currency is the dollar, but for me that I live in Peru, it is the Sol, and it is giving me an incorrect value of the goal.

image

I think the currency type should always be in dollars, because that is the currency github works with.

@danirabbit
Copy link
Member

I think the currency type should always be in dollars, because that is the currency github works with.

Ah shit. Yeah we want to localize delimiters and like the placement of the currency symbol, but not the currency type itself haha. I'll investigate more how to use this library

@alainm23
Copy link
Contributor Author

alainm23 commented Dec 6, 2024

As far as I can see, the library only works taking into account the configured regional settings.
I think we should not get so complicated and just add the dollar sign before.

@danirabbit
Copy link
Member

I think we should not get so complicated and just add the dollar sign before

Well there's that, and that's fine it can picked up by regular string translation. But we also need to localize the delimiters. In en_US there should be a , every 3 places. In some locales it's a . instead. So we need to at least format the number

@davidmhewitt
Copy link
Member

Yeah, I don't love the idea of having a token here at all, even if it only has public read only access.

The correct way to do this would probably be to have an endpoint on the website that fetches this data using a token we can keep secret and just returns the data publicly.

@davidmhewitt
Copy link
Member

Opened elementary/website#3690 to add an endpoint returning this info to the website.

@davidmhewitt
Copy link
Member

The new API endpoint is now available on the website: https://elementary.io/api/sponsors_goal

@alainm23 alainm23 requested review from tintou and danirabbit December 9, 2024 16:46
Copy link
Member

@danirabbit danirabbit left a comment

Choose a reason for hiding this comment

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

It's very cute, it works as expected, I'm happy with this. Nice work!

@danirabbit danirabbit merged commit 6f0efc9 into main Dec 9, 2024
4 checks passed
@danirabbit danirabbit deleted the alainm23/fix-329 branch December 9, 2024 18:45
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.

Show sponsor goal progress bar
5 participants