✨Amend preloading to work with branching #20611
Conversation
| * or [] if there aren't any. | ||
| */ | ||
| getNextPagesByAction() { | ||
| const actionElements = |
There was a problem hiding this comment.
nit: the name makes it seem like the type of this would be {!Array<!Element>}, where it's really !{Array<string>} (where each string is an attribute). Maybe name it actionAttrs? Or just actions?
| el => el.getAttribute('on')); | ||
|
|
||
| return actionElements.reduce((res, el) => { | ||
| if (el.includes('goToPage')) { |
There was a problem hiding this comment.
nit: Same as above; the name el makes it seem like this is an {!Element}, when it's really a {string}. Maybe action?
|
|
||
| /** | ||
| * Finds any elements in the page that has a goToPage action. | ||
| * @return {Array<string>} The IDs of the potential next pages in the story |
There was a problem hiding this comment.
Type should be non-nullable ({!Array<string>})
| } | ||
| // Do not overwrite, branching distance always takes precedence. | ||
| if (pageId !== maybePrev) { | ||
| pagesByDistance[distance].push(pageId); |
There was a problem hiding this comment.
Can this be combined with the statement below?
There was a problem hiding this comment.
I think we'd still like to go through the step of setting the distance attribute when the branching experiment is turned off!
| return actionElements.reduce((res, el) => { | ||
| if (el.includes('goToPage')) { | ||
| // The pageId is in between the equals sign & closing parenthesis. | ||
| res.push(el.slice(el.search('\=(.*)') + 1,-1)); |
There was a problem hiding this comment.
Will this still work if the element has two actions with parameters? e.g.
on="tap:foo.barAction(baz=1), story.goToPage(id=page-id)"
There was a problem hiding this comment.
Thank you for pointing this out! You're right, this won't work for that case. It also won't work for handling multiple events on="tap:story.goToPage(id=pageID);submit-error:lightbox2".
Made some changes to account for this.
There was a problem hiding this comment.
Maybe we should also add unit tests for those cases?
There was a problem hiding this comment.
Good point. I just added some tests to test out the method I wrote.
| */ | ||
| actions() { | ||
| const actionElements = | ||
| Array.prototype.slice.call(this.element.querySelectorAll('[on]')); |
There was a problem hiding this comment.
Nit: You could only query the actions that contain goToPage by doing something like querySelectorAll('[on*=goToPage]') (documentation).
Not a big deal but these selectors are pretty powerful
There was a problem hiding this comment.
Thank you for linking the documentation also, very good to know. Went ahead and did the change. I believe the inside check in the reduce for 'goToPage' still needs to be there to account for the cases outlined above (multiple events/actions).
| * @return {!Array<string>} The IDs of the potential next pages in the story | ||
| * or null if there aren't any. | ||
| */ | ||
| actions() { |
|
|
||
| story.element.querySelector('#cover').appendChild(actionButton); | ||
|
|
||
| const distanceGraph = story.getPagesByDistance_(); |
There was a problem hiding this comment.
Nit: Can we set the goToPage action in the page before the buildCallback, so we don't have to manually call this private method?
There was a problem hiding this comment.
I had some issues retrieving the distance attribute from the pages for some reason (it doesn't seem to exist during the tests on the page elements during the tests, though it's definitely being set). If it's okay with you, I think we should keep the as they are.
|
@bramanudom is this ready to be merged? |
|
@newmuis Yes! Thanks, Jon. |
* implemented changes still need to fix tests * fixed tests * removed console.logs * Add comment * addressed PR comments * fixed lint issues * Oops * revert changes * added more tests, reverted accidental change in validator.js * removed excess space that linter didnt catch * addressed nits
* implemented changes still need to fix tests * fixed tests * removed console.logs * Add comment * addressed PR comments * fixed lint issues * Oops * revert changes * added more tests, reverted accidental change in validator.js * removed excess space that linter didnt catch * addressed nits
#20083
This PR adds additional logic make preloading consider next pages that are reachable within stories through
goToPageaction, as well as previous pages that were reached through eitheradvance-toorgoToPage.For next pages we search through the page element to find the next pages. For previous pages, we make use of the story level stack (#20182).