-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Combined listings with multiple product templates #3526
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,15 +57,6 @@ if (!customElements.get('product-info')) { | |
this.postProcessHtmlCallbacks.push((newNode) => { | ||
window?.Shopify?.PaymentButton?.init(); | ||
window?.ProductModel?.loadShopifyXR(); | ||
publish(PUB_SUB_EVENTS.sectionRefreshed, { | ||
data: { | ||
sectionId: this.sectionId, | ||
resource: { | ||
type: SECTION_REFRESH_RESOURCE_TYPE.product, | ||
id: newNode.dataset.productId, | ||
}, | ||
}, | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See above |
||
}); | ||
} | ||
|
||
|
@@ -77,12 +68,14 @@ if (!customElements.get('product-info')) { | |
const productUrl = target.dataset.productUrl || this.pendingRequestUrl || this.dataset.url; | ||
this.pendingRequestUrl = productUrl; | ||
const shouldSwapProduct = this.dataset.url !== productUrl; | ||
const shouldFetchFullPage = !this.isFeaturedProduct && shouldSwapProduct; | ||
const shouldFetchFullPage = this.dataset.updateUrl === 'true' && shouldSwapProduct; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prior to this change, we were requesting a full PDP from both main-product and quick add modal, and just requesting the section for featured product. However, for the quick add modal we only need the main product section. This change makes it so that we request the full page when swapping between combined listings in a main product section, and request just the product section for both featured product and quick add modal. |
||
|
||
this.renderProductInfo({ | ||
requestUrl: this.buildRequestUrlWithParams(productUrl, selectedOptionValues, shouldFetchFullPage), | ||
targetId: target.id, | ||
callback: shouldSwapProduct ? this.handleSwapProduct(productUrl) : this.handleUpdateProductInfo(productUrl), | ||
callback: shouldSwapProduct | ||
? this.handleSwapProduct(productUrl, shouldFetchFullPage) | ||
: this.handleUpdateProductInfo(productUrl), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Explicitly pass whether we're fetching the full page to the handleSwapProduct callback |
||
}); | ||
} | ||
|
||
|
@@ -92,33 +85,27 @@ if (!customElements.get('product-info')) { | |
productForm?.handleErrorMessage(); | ||
} | ||
|
||
get isFeaturedProduct() { | ||
return this.dataset.section.includes('featured_product'); | ||
} | ||
|
||
handleSwapProduct(productUrl) { | ||
handleSwapProduct(productUrl, updateFullPage) { | ||
return (html) => { | ||
this.productModal?.remove(); | ||
|
||
// Grab the selected variant from the new product info | ||
const variant = this.getSelectedVariant(html.querySelector(`product-info[data-section=${this.sectionId}]`)); | ||
const selector = updateFullPage ? "product-info[id^='MainProduct']" : 'product-info'; | ||
const variant = this.getSelectedVariant(html.querySelector(selector)); | ||
dan-menard marked this conversation as resolved.
Show resolved
Hide resolved
|
||
this.updateURL(productUrl, variant?.id); | ||
|
||
// If we are in an embedded context (quick add, featured product, etc), only swap product info. | ||
// Otherwise, refresh the entire page content and sibling sections. | ||
if (this.dataset.updateUrl === 'false') { | ||
if (updateFullPage) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I reversed this conditional so that we can rely on the |
||
document.querySelector('head title').innerHTML = html.querySelector('head title').innerHTML; | ||
|
||
HTMLUpdateUtility.viewTransition( | ||
this, | ||
html.querySelector('product-info'), | ||
document.querySelector('main'), | ||
html.querySelector('main'), | ||
this.preProcessHtmlCallbacks, | ||
this.postProcessHtmlCallbacks | ||
); | ||
} else { | ||
document.querySelector('head title').innerHTML = html.querySelector('head title').innerHTML; | ||
|
||
HTMLUpdateUtility.viewTransition( | ||
document.querySelector('main'), | ||
html.querySelector('main'), | ||
this, | ||
html.querySelector('product-info'), | ||
this.preProcessHtmlCallbacks, | ||
this.postProcessHtmlCallbacks | ||
); | ||
|
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 was introduced as part of combined listings but is not being used anywhere. Removing to prevent locking ourselves into supporting this event (i.e. if apps start consuming it).