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
[amp-story] Refactor pagination buttons to use store ♻️ #19655
Conversation
Actually I think I'll split this into two PRs. I'll let you know when it's ready for review. |
5f3d646
to
2eced2e
Compare
</amp-story-grid-layer> | ||
</amp-story-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.
Nit: extra line break
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.
Done.
@@ -690,12 +690,10 @@ export class AmpStory extends AMP.BaseElement { | |||
return; | |||
} | |||
|
|||
this.paginationButtons_ = PaginationButtons.create(this.win); | |||
this.paginationButtons_ = | |||
PaginationButtons.create(this.win, () => this.hasBookend_()); |
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.
Plz add a TODO so we find a way to avoid passing a private function here. I understand that it's way out of the scope of this PR, and is already something we do in other places
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.
Done.
Percy LGTM |
20aaecb
to
1701584
Compare
Rebased from master to get a green build. Please review Percy again and if it looks good then it's ready to merge :) |
…9655) * move pagination buttons to use store * ensure buttons work with bookend history state * add visual tests * update pr * reviews
Closes #19600
Make pagination buttons use
AmpStoryStoreService
instead ofNavigationState
. Also adds visual tests for pagination buttons (only desktop).Follow up PRs will be made to eventually deprecate navigation state. (#19751)