-
Notifications
You must be signed in to change notification settings - Fork 9
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: APP-364 order summary card #2498
Conversation
b0d8f9f
to
30bd970
Compare
9dbd532
to
0746206
Compare
web-components/src/components/SupCurrencyAndAmount/SupCurrencyAndAmount.stories.tsx
Outdated
Show resolved
Hide resolved
web-marketplace/src/components/molecules/OrderSummaryCard/OrderSummaryCard.Content.tsx
Outdated
Show resolved
Hide resolved
0746206
to
35ed494
Compare
757e5f6
to
6627fea
Compare
✅ Deploy Preview for regen-website ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@erikalogie see testing instructions |
@erikalogie updated |
Looks good! |
@blushi Christian found this bug while testing on this branch: "Cannot change dollar amount of credits to purchase. When I do, I get an error page with an n.toFixed is not a Function message. " He tried to change the dollar amount when trying to buy fiat credits with a web 2.0 only account See this video at 12:23: https://us02web.zoom.us/rec/share/t30QW8KPjkaqmm6N3_ZLQyZMd6lhp6zgKxmoT2ct8Kfu-J4D_XYo2obx_71creby.z-JaUieYQqpeH0GX If this should be a separate bug, let me know and I can file it. |
@blushi another bug that Christian found: You'll find the issue at 18:48 in the video above. |
Yeah let's file a separate bug please |
It doesn't seem related to the buy flow itself but rather some issues related to the app state not being updated correctly immediately after logging in, since it works after reloading. Unfortunately I can't reproduce it but we should file a bug nevertheless. |
Ok I filed separate bugs for the above: https://regennetwork.atlassian.net/browse/APP-404 and https://regennetwork.atlassian.net/browse/APP-405 |
const currency = data?.[CURRENCY]; | ||
const creditsAmount = data?.[CREDITS_AMOUNT]; | ||
const currencyAmount = data?.[CURRENCY_AMOUNT]; | ||
const creditTypePrecision = creditTypeData?.creditType?.precision; |
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.
@blushi Shouldn't we memoize this as well? It is used as a dependency for creating creditsAvailable
and also passed as prop to ChooseCreditsForm
. Actually, ChooseCreditsForm
should be wrap with React.memo()
too to prevent unnecessary re-renders.
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.
we use useMemo to wrap creditsAvailable
but we can use React.memo for ChooseCreditsForm too
I was also thinking about potentially creating a BuyCreditsContext, instead of having to pass all those props to the child components, wdyt? But maybe can be refactored later.
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.
My thinking is that since creditTypePrecision
is recreated on every render, it will cause creditsAvailable
to recalculate each time. Memoizing creditTypePrecision
could prevent that, as well as unnecessary re-renders of ChooseCreditsForm
.
I like your idea about a BuyCreditsContext, it will help simplify things.
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.
but creditTypePrecision
doesn't change on every render, the actual value will only change once we get the correspond query response so this should only cause creditsAvailable
to recalcule this time (if we exclude other dependencies)
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.
I believe the issue is that the variable is recreated and it's reference changes when react re-executes the component - even if its value remains the same - and that is what can trigger a creditsAvailable
recalculation and a ChooseCreditsForm
re-render
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.
but creditTypePrecision
is a copy using some react query data which is cached and stable across re-renders (unless the data changes) so I don't think we need useMemo in this particular case
if that was another type of variable, then this would be useful indeed.
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.
You are right about the data being cached and stable across re-renders. However, my concern is that if the component re-renders for any reason, creditTypePrecision
will be recreated, even if the value hasn't changed, which may lead to unnecessary recalculations for dependent variables or components. If we repeat this pattern throughout the app it might impact performance.
That said, I’ll go ahead and approve the PR so we can move forward. Perhaps we can chat about this another time?
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.
See comments please
e21959c
to
77088c0
Compare
77088c0
to
d3efc66
Compare
Description
https://regennetwork.atlassian.net/browse/APP-364
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
How to test
https://deploy-preview-2498--regen-marketplace.netlify.app/project/C01-001/buy
Update the credits amount from the order summary card from different steps in the flow and verify the resulting amount to pay updates accordingly.
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...