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: update currency tooltips when currency is changed #845

Merged
merged 33 commits into from
Jun 22, 2021

Conversation

alfonsobries
Copy link
Member

Summary

https://app.clickup.com/t/mf6y1q

When the currency is updated on the navbar, it updates:

  1. Home transactions and block tables
  2. "View all" Transactions and block tables
  3. Wallet transactions table
  4. Block transactions table
  5. Wallet header fiat amount

Checklist

  • Documentation (if necessary)
  • Tests (if necessary)
  • Ready to be merged

@leMaur leMaur self-assigned this Jun 15, 2021
@leMaur
Copy link
Contributor

leMaur commented Jun 15, 2021

It looks good to me! 👍🏽

@alfonsobries alfonsobries changed the title feat: update currency tooltips when currency changed feat: update currency tooltips when currency is changed Jun 15, 2021
Copy link

@samharperpittam samharperpittam left a comment

Choose a reason for hiding this comment

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

@alfonsobries - Can you check something on this PR for me please? Think i've found an issue but it may be indirectly caused by this PR.

If you go to the following URL /wallets/DFyLKkWs12QwDTi8BywQN5ssa5CMK3dr6d and then change the currency within the navbar.

Do the icons in the tx history table get messed up for you?

image

@alfonsobries
Copy link
Member Author

@samharperpittam ✅ fixed

@ItsANameToo
Copy link
Member

@alfonsobries would it be possible to refresh the tooltips without having the whole table go into a loading state? feels odd to see the full table reload when the currency changes as it doesn't affect anything that's visible in the table at that point, or is it me @samharperpittam 🤔

@alfonsobries
Copy link
Member Author

alfonsobries commented Jun 18, 2021

@ItsANameToo yeah probably possible. Ill work on that right now

@alfonsobries alfonsobries marked this pull request as draft June 18, 2021 14:40
@alfonsobries
Copy link
Member Author

@ItsANameToo thought the change related to the skeleton would be very easy but it wasn't

Since the skeleton is needed for the pagination OR for when we update the filter the solution is to find a way to show the skeleton conditionally, unfortunately, I didn't find a way an I tried a lot of alternatives.

For example, I tried to add a flag to "disable" the skeleton but if I update the flag that triggers an update, and livewire shows the skeleton so I am running on circles here.

Other alternatives I can think means changing the current implementation of the skeleton (make a JS only skeleton for example) but that means a big refactor, maybe for another PR

@ItsANameToo
Copy link
Member

let's leave it as-is then @alfonsobries . We can recheck this if we get complains from users as it's limited to changing the currency and generally you don't change that every 2 minutes anyway

@ItsANameToo
Copy link
Member

@alfonsobries conflicts

@alfonsobries alfonsobries marked this pull request as ready for review June 22, 2021 10:53
@ItsANameToo ItsANameToo merged commit 16c1a45 into develop Jun 22, 2021
@ItsANameToo ItsANameToo deleted the feat/update-currency-stuff-when-changed branch June 22, 2021 12:41
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.

None yet

5 participants