-
Notifications
You must be signed in to change notification settings - Fork 683
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
Removes ability to add configurable items to cart until options are chosen #1097
Merged
dpatil-magento
merged 8 commits into
develop
from
supernova/640-configurable_product_error
Apr 4, 2019
Merged
Removes ability to add configurable items to cart until options are chosen #1097
dpatil-magento
merged 8 commits into
develop
from
supernova/640-configurable_product_error
Apr 4, 2019
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This pull request is automatically deployed with Now. |
tjwiebell
previously approved these changes
Apr 3, 2019
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.
Have a suggestion for improvement, but this would be fine as-is. Merge away if you'd prefer not to introduce __typename
into the mix.
packages/venia-concept/src/components/ProductFullDetail/ProductFullDetail.js
Outdated
Show resolved
Hide resolved
- Is now based on the product __typename - No longer checks if configurable_items is an array
PR Updated:
|
tjwiebell
approved these changes
Apr 4, 2019
sirugh
added
the
version: Minor
This changeset includes functionality added in a backwards compatible manner.
label
May 24, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
This PR disables the "Add to Cart" button on configurable product description pages (PDPs) until all product options have been selected.
Related Issue
Closes #640 .
Verification Steps
Configurable Products
Non-Configurable Products
How Have YOU Tested this?
I loaded up a configurable and non-configurable product, as described above.
yarn test
passes.Screenshots / Screen Captures (if appropriate):
Proposed Labels for Change Type/Package
Checklist:
Further Details / Risks / Notes / Etc.
This is perhaps the simplest solution to this problem. Some other options would be:
Client-side, per-field Validation
Other forms in the app (ex: Address Form) have the ability to perform client-side, per-field validation and show error messages inline on the form.
Unfortunately the PDP would need to be heavily refactored to allow for this, and a solution for the color swatches would need to be included as part of this (changing them to radio buttons instead of actual buttons).
Why I didn't go this route:
We would still want to stick to Venia's precedent and pattern of not enabling the "submit" button of the form until all required fields are filled out, and since there is no additional client-side validation on top of this field was filled in, the refactor seemed to be too much work for not enough payoff.
Edit: turns out this isn't much of a precedent or pattern. The checkout flow doesn't enable the submit button until billing, payment, and shipping are filled in but other forms have their submit buttons enabled and hooked up to client-side validation first.
Capturing the Error in Redux
Currently our backstop reducer handles the error from the server that results from attempting to add a configurable item to your cart without specifying options.
Ideally, the
cart
reducer handles this itself and passes the error back up to the UI (via Redux andmapStateToProps
) so that the UI can show a nice, in-context error message.Why I didn't go this route:
By disabling the "Add to Cart" button until all options are selected, we sidestep this problem.
Because client-side validation can be circumvented, some consideration should be given to the question of "what happens if the call to the server is made some other way?", in which case the backstop reducer would still kick in and the original issue would persist.
But I don't think solving for this case is worth it: if someone intentionally sidesteps our UI to make a server call, they shouldn't expect a nice clean UI if it fails.