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.
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
[Hooks integration] Checkout Payment Section + Checkout Confirmation Page #1075
[Hooks integration] Checkout Payment Section + Checkout Confirmation Page #1075
Changes from 28 commits
8178130
e72e43e
d266c0b
f03735c
858903a
a61cd1e
92668e0
108da50
6723c47
11af7c9
cfaf287
e205e18
23a7976
f3d6fe0
fe6bc8a
2033968
47ddb56
e3ad4db
97b494e
74f44eb
5bf30f0
98bdd53
edafe63
8fd4c65
b6aee0b
a5af363
81df103
208585a
353d014
358eda9
df0fa11
c0a1644
8c23d48
49c39fb
e31cf79
44a8f94
fb31c24
5fc4636
ead8ccb
595c286
b8af41c
cb6a6b8
e889214
1be9e1e
39ef577
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.
If we check loading here, we won't need to guard basket below
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.
loading is always false here since it is checked on line 310
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.
if it is false, it indicates that basketData should be defined, we technically don't need to guard the basket here. Guarding is a good way to make sure things don't break, but if we know we have guarded other cases, we should not throw in the ? too much. 🤔
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.
sounds good, removed the guard
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.
@alexvuong @kevinxh FYI it looks the change broke the cart page when the cart is empty.
http://localhost:3000/global/en-GB/cart
This is a regression that was fixed in this PR: #1062
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.
ok... i'm just gonna revert the change in cart.js.... it's out of scope for this PR and i thought it's a small diff improvement. but turns out to be too much hassle
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.
It's cool now the confirmation page is appending the order id to the url.