-
Notifications
You must be signed in to change notification settings - Fork 23
Splits Delivery and Payment Checkout Steps #169
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: master
Are you sure you want to change the base?
Conversation
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.
Line exceeds maximum allowed length
|
@dw Looks good 👍 |
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.
Line contains inconsistent indentation
|
CI builds (other than Hound) are not supposed to pass, right? |
|
@joshnuss Any issues merging this? |
|
Actually, hold pls. |
|
The only bit that makes me slightly uncomfortable passing in an Thoughts? |
|
@joshnuss Much as I appreciate the complement, I wish I didn't receive quite as much random GitHub mail of this sort :) |
|
@DanielWright if you've tested it, it looks good to merge (sorry I keep using the wrong username) |
|
Hah, it's a bit confusing since I go by DW everywhere else. (Sorry, the-real-dw.) Rod and I are working through the UX implications in one of our host apps, I'm going to leave this PR unmerged until we are feeling a bit more comfortable. |
|
How do you feel about where the UX is at for this? |
|
@DanielWright I'm not sure we have got to a consensus regarding the UX for the delivery-payment split yet. |
Several Spree modules rely on certain checkout-transition callbacks being triggered during checkout. As a result of smushing delivery and payment steps together, Sprangular inadvertently skips these callbacks, which causes some weird behaviours. This commit attempts to correct this issue by performing the two steps discretely.
d7e6b58 to
899adda
Compare
|
Same question as last time. D'you think we're close enough on UX now that we can get this integrated and mergeable? |
This commit tweaks the view's UX in order to remove the onSelection method from the shipping rates list, requiring the delivery step to be manually advanced by the user.
|
@DanielWright I've tweaked the checkout view's UI/UX in order to reflect the step splitting. I'll rebase it to #63 once the PR is merged to master. |
The current UX is to return the user with a popup when the data fails to load and there is a routeChangeError. This is a bit aggressive and confusing to the user. This commit changes this behaviour to logging a message in the console and redirecting to a 404 page, as seen in #198.
Several Spree modules rely on certain checkout-transition callbacks being triggered during checkout. As a result of smushing delivery and payment steps together, Sprangular inadvertently skips these callbacks, which causes some weird behaviours. See #168 for additional details.
This commit attempts to correct this issue by performing the two checkout steps discretely.