-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Update main product sibling sections when switching between combined listing children #3368
Update main product sibling sections when switching between combined listing children #3368
Conversation
d632f13
to
c760ec5
Compare
ae7a814
to
731a790
Compare
f531545
to
2a22f8a
Compare
); | ||
|
||
document.querySelectorAll('main > .shopify-section').forEach((section) => { | ||
if (!section.id.includes('__main') && updatedSections[section.id]) { |
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.
@tyleralsbury / @tauthomas01 I'm not sure how section IDs are generated.... is it a safe assumption to make that the section IDs will match between the initial product and the sibling products?
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.
Section id is generated via the JSON template so it should be fine.
In this case, __main
is generated from the product.json
templates. These are section identifier that you can set in the template.
Here's an example of collection page:
.then((response) => response.text()) | ||
.then((responseText) => { | ||
const html = new DOMParser().parseFromString(responseText, 'text/html'); | ||
const updatedSections = Array.from(html.querySelectorAll('main > .shopify-section')).reduce( |
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.
@tyleralsbury / @tauthomas01 It looked like selecting shopify-section
s under main
targets all sections that could contain a product metafield (and so would need to be updated). Is that correct?
} | ||
}); | ||
}); | ||
} |
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 function is pretty naive and assumes that sibling products of a combined listing will use the same product template. If the product sections differ the UX will be pretty poor.
On the other hand, the primary use case for combined listings is to combine many products to appear as a single product. Because of this, the functionality between combined listing children should be identical in most cases, and children having different content should be a slim edge case.
Given that this isn't a one-way door (e.g. merchants can update theme code to accommodate multi-template CLs) and that it is an edge case, our designer on combined listings thought this was an acceptable tradeoff.
What do you think?
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 think this is a reasonable assumption. Is there anyway to detect if the template changes between child products and print a warning or anything?
I'm assuming this would manifest by updates not working or looking wonky, and I could see it being confusing/frustrating to try and figure out what is going on if you don't realize products have different templates or know that is a limitation.
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.
Yeah good idea, maybe we add a console.error that mentions that's what's happening here 🤔
20510e2
to
2a22f8a
Compare
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.
Looks good, tophat checks out for me. Thanks for the good explanations and annotations.
} | ||
}); | ||
}); | ||
} |
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 think this is a reasonable assumption. Is there anyway to detect if the template changes between child products and print a warning or anything?
I'm assuming this would manifest by updates not working or looking wonky, and I could see it being confusing/frustrating to try and figure out what is going on if you don't realize products have different templates or know that is a limitation.
#handleSwapProduct() { | ||
return (html) => { | ||
this.productModal?.remove(); | ||
const newProduct = html.querySelector('product-info'); | ||
this.swapProductUtility.viewTransition(this, newProduct); | ||
this.relatedProducts?.initializeRecommendations(newProduct.dataset.productId); | ||
this.quickOrderList?.refresh(); |
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 assume these sections are covered by the overall updateProductSections
now?
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.
right exactly
); | ||
|
||
document.querySelectorAll('main > .shopify-section').forEach((section) => { | ||
if (!section.id.includes('__main') && updatedSections[section.id]) { |
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.
Section id is generated via the JSON template so it should be fine.
In this case, __main
is generated from the product.json
templates. These are section identifier that you can set in the template.
Here's an example of collection page:
}); | ||
}); | ||
} | ||
|
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.
TODO this function should publish the section_refreshed event once complete
831e093
to
45f65d3
Compare
…election is changed to a sibling
2a22f8a
to
7e35aa4
Compare
474be3d
to
4f3b617
Compare
Closing, this will be fixed by the changes introduced here: #3289 Ultimately, the approach that combined listings takes for swapping product content is:
|
PR Summary:
All child sections of a main product may contain references to the product through metafields. Given this, we want to ensure that all sections of the main product are refreshed when switching between child products.
Demo video of error: https://screenshot.click/23-29-tjpdt-m09nb.mp4
What approach did you take?
Chatted with @tyleralsbury , the recommendation was to request a full page update concurrently with the section api call to update the product info section, then use the response to update other sections. (The full page request seems to take anywhere from a few to a few hundred MS longer than the product section request so it likely makes sense to keep this as a separate request.)
Other considerations
refreshable-section
component that listens for an event and self-refresh.product-info.js
could declaratively update all sections associated with the productVisual impact on existing themes
This will only impact combined listings products
Demo links
Checklist