-
Notifications
You must be signed in to change notification settings - Fork 1
Provision reserved balances and the useAccountBalances hook #829
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
base: group/transactions-v2
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @glad-adyen, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the handling of account balances by introducing a dedicated Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new useAccountBalances hook to encapsulate the logic for fetching account balances, which is a great improvement for code organization and reusability. The Balances component is refactored to use this new hook, and mock data is updated to include reserved balances. My review includes a few points to improve the implementation. In Balances.tsx, there's an issue with using non-stable keys for list items, and the reservedValue for balances seems to be dropped, preventing it from being displayed. Additionally, the new useAccountBalances hook is missing unit tests, and some analytics tracking logic has been removed in useBalanceAccountSelection.ts, which might be unintentional. Overall, the changes are positive, and addressing these points will make the PR even better.
src/components/external/TransactionsOverview/components/Balances/Balances.tsx
Outdated
Show resolved
Hide resolved
src/components/external/TransactionsOverview/components/Balances/Balances.tsx
Show resolved
Hide resolved
4f8f0de to
fba493c
Compare
fba493c to
d173eff
Compare
88e992f to
1dfc0b3
Compare
1dfc0b3 to
ed22549
Compare
Summary
This PR introduces the following changes to the functionality for getting and rendering account balances.
useAccountBalanceshook to encapsulate the entire logic for fetching account balances.<Balances />component to use the newuseAccountBalanceshook.reservedValuefield indicating the reserved balance amount.In addition, this PR also reverts the
useBalanceAccountSelectionhook to its previous state before the addition of analytics tracking. This was done to remove a tracking event that was wrongly being sent from the hook. In the near future, this event will be reintroduced, but in more appropriate location(s).Fixed issue: IEX-1220: Update mock data for account balances endpoint