✨ [amp story shopping] Product description page template (PDP) #37535
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
processprocess
requested review from
mszylkowski,
jshamble,
gmajoulet and
coreymasanto
and removed request for
gmajoulet
|
Hey @gmajoulet! These files were changed: Hey @newmuis! These files were changed: |
gmajoulet
reviewed
Feb 8, 2022
extensions/amp-story-shopping/0.1/amp-story-shopping-attachment.css
Outdated
Show resolved
Hide resolved
extensions/amp-story-shopping/0.1/amp-story-shopping-attachment.css
Outdated
Show resolved
Hide resolved
| @@ -105,45 +109,213 @@ export class AmpStoryShoppingAttachment extends AMP.BaseElement { | |||
| } | |||
| this.storeService_.subscribe( | |||
| StateProperty.PAGE_ATTACHMENT_STATE, | |||
| (isOpen) => this.onPageAttachmentStateUpdate_(isOpen) | |||
| (isOpen) => { | |||
| const shoppingData = this.storeService_.get( | |||
There was a problem hiding this comment.
I know you're checking if (isOnActivePage) in all the methods, but I wouldn't mind either moving the check here, or duplicating the check if you want to be super safe (or have other code paths calling these methods)
extensions/amp-story-shopping/0.1/amp-story-shopping-attachment.js
Outdated
Show resolved
Hide resolved
Comment on lines
171
to
204
| // Iterate over built templates and set active attribute. | ||
| // If a template is already built, return early. | ||
| let templateAlreadyBuilt = false; | ||
| Object.values(this.builtTemplates_).forEach((template) => { | ||
| if (template === this.builtTemplates_[templateId]) { | ||
| templateAlreadyBuilt = true; | ||
| this.mutateElement(() => { | ||
| template.setAttribute('active', true); | ||
| this.resetCarouselScroll_(template); | ||
| }); | ||
| } else { | ||
| this.mutateElement(() => template.removeAttribute('active')); | ||
| } | ||
| }); | ||
|
|
||
| // Early return if template is already built. | ||
| if (templateAlreadyBuilt) { | ||
| return; | ||
| } | ||
|
|
||
| // If not already built, build template and assign it to builtTemplates_ by templateId. | ||
| this.builtTemplates_[templateId] = ( | ||
| <div class="i-amphtml-amp-story-shopping" active> | ||
| {featuredProduct && this.renderPdpTemplate_(featuredProduct)} | ||
| {shoppingDataForPage.length > 1 && | ||
| this.renderPlpTemplate_( | ||
| shoppingDataForPage.filter((item) => item !== featuredProduct) | ||
| )} | ||
| </div> | ||
| ); | ||
|
|
||
| this.mutateElement(() => | ||
| this.templateContainer_.appendChild(this.builtTemplates_[templateId]) | ||
| ); |
There was a problem hiding this comment.
This piece of code is mostly great but has a slight design issue, it creates two code paths depending on if the template is created or re-used.
Down the line this creates more complexity, e.g. you only resetCarouselScroll when re-using the template, not creating it. If you scroll on a PLP and click on a product, the scroll reset behavior would already be different.
You also shouldn't set active directly on the template as this is an option, if you wanted to prerender the templates that wouldn't work out of the box.
You could extract the template creation into a separate method, like
// pseudocode
getTemplate(templateId, featuredProduct, shoppingDataForPage) {
return this.builtTemplates_[templateId] || (jsx);
}
And then in your updateTemplate:
// Pseudocode
updateTemplate(...) {
...
forEach(template => removeAttr('active'));
const template = getTemplate(...);
// append template if !template.isConnected, set template active, resetScroll
}
There was a problem hiding this comment.
isConnected 🔥 TIL.
coreymasanto
reviewed
Feb 8, 2022
extensions/amp-story-shopping/0.1/amp-story-shopping-attachment.js
Outdated
Show resolved
Hide resolved
Comment on lines
191
to
204
| // If not already built, build template and assign it to builtTemplates_ by templateId. | ||
| this.builtTemplates_[templateId] = ( | ||
| <div class="i-amphtml-amp-story-shopping" active> | ||
| {featuredProduct && this.renderPdpTemplate_(featuredProduct)} | ||
| {shoppingDataForPage.length > 1 && | ||
| this.renderPlpTemplate_( | ||
| shoppingDataForPage.filter((item) => item !== featuredProduct) | ||
| )} | ||
| </div> | ||
| ); | ||
|
|
||
| this.mutateElement(() => | ||
| this.templateContainer_.appendChild(this.builtTemplates_[templateId]) | ||
| ); |
There was a problem hiding this comment.
Mind pulling this out into a new buildTemplate_(templateId, featuredProduct, shoppingDataForPage) method?
gmajoulet
approved these changes
Feb 8, 2022
| @@ -105,45 +109,213 @@ export class AmpStoryShoppingAttachment extends AMP.BaseElement { | |||
| } | |||
| this.storeService_.subscribe( | |||
| StateProperty.PAGE_ATTACHMENT_STATE, | |||
| (isOpen) => this.onPageAttachmentStateUpdate_(isOpen) | |||
| (isOpen) => { | |||
| const shoppingData = this.storeService_.get( | |||
This was referenced Feb 9, 2022
This was referenced Feb 10, 2022
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.
Demo
The details module will be integrated in #36740