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

feat: added NGN currency #1264

Merged
merged 1 commit into from
Jul 21, 2024
Merged

feat: added NGN currency #1264

merged 1 commit into from
Jul 21, 2024

Conversation

aniskhalfallah
Copy link
Contributor

@aniskhalfallah aniskhalfallah commented Jul 15, 2024

Description

Closes #1226
Added Naira currency.

Screenshots

proof_ngn-stacker news

Additional Context

Checklist

Are your changes backwards compatible? Please answer below:

Yes, by deleting the line that adds the Naira Currency

Did you QA this? Could we deploy this straight to production? Please answer below:

Yes

For frontend changes: Tested on mobile? Please answer below:

N/A

Did you introduce any new environment variables? If so, call them out explicitly here:
N/A

Copy link
Member

@ekzyis ekzyis left a comment

Choose a reason for hiding this comment

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

Sorry for the slow review, works well!

A nitpick (which was already the case prior to this PR) is that we show the new currency symbol with the old value until the new data price data is fetched. This can take up to 30s.

I briefly looked into refetching it immediately using useEffect in PriceProvider but since we do the price lookup asynchronously in the backend, it requires more changes than I expected for such a small detail.

@aniskhalfallah
Copy link
Contributor Author

aniskhalfallah commented Jul 18, 2024

Hello @ekzyis Thanks for the review !
I noticed the issue after you mentioned it, I looked into it by using useEffect to refetch after changing the currency:

useEffect(() => {
    if (fiatCurrency) {
      refetch({ fiatCurrency });
    }
  }, [fiatCurrency, refetch]);

I tested it, and in my requests I see a query 1 second after I change the currency

useEffect
useEffect2
useEffect3

I am still a noob so I am not sure whether this fixes the issue or not since you said it requires more changes, Let me know what you think.

@huumn huumn merged commit f051aff into stackernews:master Jul 21, 2024
9 checks passed
aniskhalfallah added a commit to aniskhalfallah/stacker.news that referenced this pull request Aug 8, 2024
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.

Addition of Naira(NGN) to fiat currencies
3 participants