Skip to content

Fixes amp-story-shopping shopping tag not showing up on refresh when using remote data#37793

Merged
processprocess merged 4 commits intoampproject:mainfrom
jshamble:fixshoppingTagDataII
Mar 2, 2022
Merged

Fixes amp-story-shopping shopping tag not showing up on refresh when using remote data#37793
processprocess merged 4 commits intoampproject:mainfrom
jshamble:fixshoppingTagDataII

Conversation

@jshamble
Copy link
Copy Markdown
Contributor

@jshamble jshamble commented Feb 28, 2022

Closes #37749

There is an issue with the shopping data: the shopping tags subscribe to changes to the shopping data in the store service. However, it may not always be the case that the correct data will be loaded for the tag as sometimes the promises are unresolved, such as the case with remote data.

To fix this problem, we have added an additional conditional check for making sure that the data is loaded first before rendering. We have also refactored it so it only adds the shopping tag listeners after a valid shopping tag is created, not in the layout callback.

@amp-owners-bot
Copy link
Copy Markdown

Hey @gmajoulet! These files were changed:

extensions/amp-story-shopping/0.1/amp-story-shopping-tag.js

@processprocess
Copy link
Copy Markdown
Contributor

Very nice.
We could refactor this to only attach the listeners when we need them. Check it out.

This way the logic only runs if the tag exists. It removes null checks and the calltoinitialize will set the initial state of the tag.

@processprocess
Copy link
Copy Markdown
Contributor

processprocess commented Mar 1, 2022

It would be helpful to rename this to maybeCreateAndAppendInnerShoppingTagEl_ since it doesn't always create the shopping tag.

(shoppingData) => this.createAndAppendInnerShoppingTagEl_(shoppingData),

@jshamble jshamble requested a review from processprocess March 1, 2022 21:57
@jshamble jshamble requested a review from processprocess March 1, 2022 22:24
@processprocess processprocess merged commit c980308 into ampproject:main Mar 2, 2022
@ampprojectbot
Copy link
Copy Markdown
Member

Warning: disparity between this PR Percy build and its main build

The Percy build for this PR was approved (either manually by a member of the AMP team, or automatically if there were no visual diffs). However, during a continuous integration step we generated another Percy build using the commit on the main branch that this PR was merged into, and there appears to be a mismatch between the two.

This is possibly an indication of an issue with this pull request, but could also be the result of flakiness. Please inspect the two builds < This PR's Percy build / main commit's Percy build > and determine further action:

  • If the disparity appears to be caused by this PR, please create an bug report or send out a new PR to fix
  • If the disparity appears to be a flake, please @-mention ampproject/wg-approvers in a comment
  • If there is no disparity and this comment was created by mistake, please @-mention ampproject/wg-infra
  • If unsure, @-mention ampproject/wg-approvers

@processprocess
Copy link
Copy Markdown
Contributor

processprocess commented Mar 2, 2022

@ampproject/wg-approvers This appears to be a flake caused by amp-video which is unrelated to this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[amp story shopping config] Error when loading story on page that uses remote data

3 participants