✨Add a go to page action #20084
✨Add a go to page action #20084
Conversation
| this.registerAction('goToPage', invocation => { | ||
| const {args} = invocation; | ||
| if (args) { | ||
| this.switchTo_(args['targetPageId']); |
| win.document, | ||
| 'button', | ||
| {'id': 'actionButton', | ||
| 'on': 'tap:cover.goToPage(id=page-2)'}); |
There was a problem hiding this comment.
Just a question about how publishers will have to use this: it'll have to be tap:${pageId}.goToPage(...)?
Is there a way to make it so they don't have to know the current pageId to write the action?
There was a problem hiding this comment.
Yes, this is correct! Unfortunately, because we want this to be an amp action, based on the actions and events documentation, the targetId is required.
There was a problem hiding this comment.
Can't the ID be the ID of the button?
There was a problem hiding this comment.
Would it be feasible as a future change to implement the action on the <amp-story> element to avoid needing to reference the specific page?
There was a problem hiding this comment.
@newmuis I believe that the targetId should be the dom element that we'd like to execute an action on. The go to action really just switches the page that is currently active which is handled by the amp-story-page element (and then at the amp-story level). I think the action needs to be bound to a page element. Just to be sure, I did a quick test and it doesn't seem to work with the id of the button.
@cvializ I think that would be possible, since the switching method is handled both in the story and page level. Though, I'm not sure how we feel about the fact that this would require giving the amp-story element an id (would users typically do this?), whereas a pageId is always required to make a story.
There was a problem hiding this comment.
Whoops, makes sense. One data point here is that amp-carousel has a similar API ("go to slide") which uses the carousel ID. I think semantically the story ID may make more sense ("the story should go to page 5" rather than "page 1 should go to page 5")
| .addAdvanceListener(() => this.next(/* opt_isAutomaticAdvance */ true)); | ||
| this.advancement_ | ||
| .addProgressListener(progress => this.emitProgress_(progress)); | ||
| this.registerAction('goToPage', invocation => { |
There was a problem hiding this comment.
I just realized: we can't keep this from going live with validation, because the on="..." syntax is always valid. Maybe we should add an experiment to keep this from launching before we're ready?
See #19699 as an example of setting up an experiment.
|
You'll need to enable the experiment in your unit test now, and potentially set it back to |
|
🎉 |
* added the go to page action * added tests * fixed tests * changed action argument name * go to action should be bound to the story and not the page * enable amp-story-branching experiment * oops, commas to separate arguments is a good thing * fixed tests * re-add removed line
#20083
Clicking on an element with this action should bring the user to another page within the same story.
Note that there will be a follow-up PR to handle changes to navigation, per UX requirement . Currently, after jumping to a new non-adjacent page, navigating to the previous page does not bring you back to the page you jumped from.