🐛 Quick order list and combined listings #3373
Closed
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
…ing product-info class and migrating product-update specific logic out of the VariantSelects class. A product wrapper enables children to more trivially perform global updates by providing a heirarchical "namespace"--i.e. child publishes event, parent captures and declaratively updates other children VS child updates siblings. By extracting the VariantSelects onChange logic to use this pattern, VariantSelects is able to be a single-purpose class, it is easier to understand why siblings are updated, and we were able to eliminate some really gross logic that handled variant change updates differently depending on the wrapping context.
…election is changed to a sibling
lhoffbeck
commented
Mar 26, 2024
There was a problem hiding this comment.
I highly recommend hiding whitespace to make this diff easier to understand
| confirm: 'confirm', | ||
| remove: 'remove', | ||
| cancel: 'cancel' | ||
| if (!customElements.get('quick-order-list-remove-button')) { |
There was a problem hiding this comment.
We actually already have these changes on main . It was from the quick add bulk PR
https://github.com/Shopify/dawn/blob/main/assets/quick-order-list.js#L1
I recommend rebasing off of main
There was a problem hiding this comment.
oh amazing 🙏 🙏 🙏 🙏
There was a problem hiding this comment.
Ill review again when it's updated :)
I also see this PR https://screenshot.click/28-40-bgg2h-6n5o1.png
has conflicts that need to be fixed #3289
9 tasks
lhoffbeck
force-pushed
the
lh-prototype-combined-listing-pdp-section-updates
branch
from
2a22f8a
to
7e35aa4
Compare
|
Closing, this has been fixed by a few other PRs |
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.
PR Summary:
Fixes JS error when reloading the quick order list section and titles for combined listings products
What approach did you take?
JS error
Variant titles
has_only_default_variantto determine whether a variant is default or not, becausehas_only_default_variantstill returnsfalseif a combined listing is made up of child products that each only have a single variant. For example, if we have a combined listing "Pants" that has children "Pants - blue", "Pants - green" where both blue and green have no additional options, the product has 2 total variants where each is a default variant.is_default_variantvalue. This also enabled us to simplify quick order list row input to just take a variant instead of a product or a variant.Default titleinstead of the product title.Visual impact on existing themes
Combined listing parent
Before
After
Combined listing child
Before
After
Product with options
Before
After
Product with only default variant
Before
After
Demo links
Links
Steps
with PR changes, theme before the changes is calledwithout PR changesUncaught SyntaxError: Identifier 'QuickOrderListRemoveButton' has already been declared (at quick-order-list.js:1:1)when switching optionsChecklist