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.

Sign up for GitHub

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

Jump to bottom

Feat/1381 notifications fiat #1430

Merged
merged 19 commits into from
Sep 25, 2022
Merged

Feat/1381 notifications fiat #1430

merged 19 commits into from
Sep 25, 2022

Conversation

escapedcat
Copy link
Contributor

@escapedcat escapedcat commented Sep 13, 2022 •
edited
Loading

Describe the changes you have made in this PR

  • show fiat in notifications
  • refactor rate to be sats- instead of a btc-rate to work directly with sats everywhere

Link this PR to an issue

Fixes #1381

Type of change (Remove other not matching type)

  • feat: New feature (non-breaking change which adds functionality)

Screenshots of the changes

  • with fiat
    image
  • no fiat
    image

How has this been tested?

  • manually (see screenshots)
  • unit-test

@escapedcat escapedcat changed the base branch from master to feat/1021_app-cache September 13, 2022 06:06
@github-actions
Copy link

github-actions bot commented Sep 13, 2022 •
edited
Loading

🚀 Thanks for the pull request!

Here are the current build files for testing:

Download and unzip the file for your browser. Refer to the readme for detailed install instructions.

This build is brought to you by: null (who recently dropped 210 sats):

Checkout the Alby Lightning Mixtape: https://alby-mixtape.vercel.app - happy coding!

Want to sponsor the next build? send some sats to ⚡️builds@getalby.com (don't forget to provide your name)

Don't forget: keep stacking sats!

@escapedcat escapedcat force-pushed the feat/1381_notifications-fiat branch from 446343c to 16c4e36 Compare September 13, 2022 09:37
@escapedcat escapedcat force-pushed the feat/1021_app-cache branch 5 times, most recently from 38ca891 to ab3ed4d Compare September 18, 2022 09:44
Base automatically changed from feat/1021_app-cache to master September 21, 2022 08:41
@escapedcat escapedcat force-pushed the feat/1381_notifications-fiat branch 2 times, most recently from 282a410 to bd25d6f Compare September 22, 2022 09:19
@escapedcat escapedcat marked this pull request as ready for review September 22, 2022 09:29
@escapedcat escapedcat force-pushed the feat/1381_notifications-fiat branch from bd25d6f to a5b56ed Compare September 22, 2022 12:13
bumi
bumi approved these changes Sep 23, 2022
View reviewed changes
Copy link
Collaborator

@bumi bumi left a comment

Choose a reason for hiding this comment

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

utACK

did you test it with currencies where the values are different? (e.g. different amount of decimal points or values?

src/common/utils/currencyConvert.ts Outdated Show resolved Hide resolved
@escapedcat
refactor(currencyconvert): use localstorage for rate to #1020
509d33b 
- avoid multiple calls per screen
@escapedcat
refactor(currencyconvert): update types, remove old code #1021
6a1781c
@escapedcat
refactor(currencyconvert): remove isLatestRate and update tests #1021
433f8db
@escapedcat
test(cache): getCurrencyRateFromCache #1021
283760c
@escapedcat
refactor(currencyrate): move functionality into background-action #1021
0f6bd52
@escapedcat
chore: cleanup and types #1021
b4d9378
@escapedcat
chore: cleanup #1021
72ef196
@escapedcat
test(cache): update test to match action #1021
33fa559
@escapedcat
test(currencyConvert): update test to match action #1021
b6fb4c4
@escapedcat
fix(cache): check if currency changed #1021
f5b1fc5
@escapedcat
feat(notifications): show fiat value for payment #1381
1a08332
@escapedcat
test(notifications): skip tests for now because spys will not work an…
70048fd 
…ymore #1381
@escapedcat
feat(notifications): check showFiat #1381
b4f8bfd
@escapedcat escapedcat force-pushed the feat/1381_notifications-fiat branch from a5b56ed to 235ee4c Compare September 23, 2022 09:42
@escapedcat
Copy link
Contributor Author

escapedcat commented Sep 23, 2022 •
edited
Loading

did you test it with currencies where the values are different? (e.g. different amount of decimal points or values?

No, thanks!
Currently this would be cut off:
image

We could move Fiat to next line:
image

@reneaaron @im-adithya what do you think?

@im-adithya
Copy link
Member

Maybe something like this:
✅ Payment [to XYZ] Successful
Amount: 321 sats (25.73 NGN)
Fee: 0 sats

@escapedcat
Copy link
Contributor Author

Maybe something like this: ✅ Payment [to XYZ] Successful Amount: 321 sats (25.73 NGN) Fee: 0 sats

Good idea, let me try if simple line-breaks work in there

@escapedcat escapedcat merged commit bbe56b8 into master Sep 25, 2022
@escapedcat escapedcat deleted the feat/1381_notifications-fiat branch September 25, 2022 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bumi bumi bumi approved these changes

@im-adithya im-adithya im-adithya approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Show fiat rates in notifications
3 participants
@escapedcat @im-adithya @bumi