This repository has been archived by the owner on Feb 23, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Update Express Payments Loading UI #4228
Update Express Payments Loading UI #4228
Changes from 21 commits
c4a6dae
df3a5db
8046fbb
b89adac
2a45339
aaefe62
1a47376
e4c006f
c88dfa6
47e005a
c28052e
ff03baa
a93e800
df6149f
80203c9
2647867
05cd597
1c0bce4
2585395
3ab9be8
df23858
8a7e07d
989f02e
5dc39a8
b8ba959
fb51e95
9d34dae
7eb865b
407ab91
dc19123
c66854a
a8c4905
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This is a significant change and testing notes don't cover it (and afaict there are no automated tests covering this - although I could be mistaken). Should there be some mention of coverage in testing notes and/or add some automated tests around behaviour?
I do see how this improves the memoization of
dispatchCheckoutEvent
given the high volatility of store cart value. On the surface this seems like there shouldn't be any unintended side-effects of the change given cartParam should always reflect the current store state whendispatchCheckoutEvent
is invoked.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 know why I did this (it's used as a dependency in various places and I didn't want to trigger the effects when the cart changed).
The change was to store the cart as a ref so the callback function can use the ref instead of using cart as a dependency. I did a quick test to make sure I understand refs and callbacks so it will have the latest cart object available (https://codesandbox.io/s/clever-brook-i8fuy?file=/src/app.js).
I know tests would be good, but I'm still not confident writing tests from scratch and I know it will take me the rest of the week to fathom.
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've restored the
storeCart
parameter name here; that was an oversight on my part. It's still using the ref.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'm okay with it not blocking the PR, but I do think it'd be worthwhile doing this as a followup. We need more confidence when changing things and automated tests are a good way to get that.