Deduplicates amp-story-page ids. #24719
Conversation
|
Hey @gmajoulet, these files were changed:
Hey @newmuis, these files were changed:
|
gmajoulet
force-pushed
the
dedupe_page_ids
branch
from
183a96b
to
365b04b
Compare
| */ | ||
| initializePageIds_() { | ||
| const pageEls = this.element.querySelectorAll('amp-story-page'); | ||
| const pageIds = Array.prototype.map.call(pageEls, el => el.id || 'default'); |
There was a problem hiding this comment.
nit: WDYT about 'page' (or default-page, or similar) as the default?
| await story.layoutCallback(); | ||
|
|
||
| expect(story.storeService_.get(StateProperty.PAGE_IDS)).to.deep.equal([ | ||
| 'cover', |
There was a problem hiding this comment.
This feels like the right behavior to me, and I trust the tests more than myself, but I don't think I get how this is getting cover/cover__1 instead of cover__1/cover__2
There was a problem hiding this comment.
It's due to the condition set in the while ((index = pageIds.indexOf(pageIds[i], i + 1)) !== -1) { of line 531 in amp-story.js.
the index will take the value of the first duplicate it finds after the current i element. Thus, the current element remains unchanged.
Perhaps there is a way to make that loop a bit easier to read.
There was a problem hiding this comment.
The second parameter of indexOf gives a start index. Goal was to reduce the O(n2) complexity since indexOf will loop over the array.
I was trying to do as little updates as possible, which is why it's keeping the first id as cover and not cover__0. I thought it made more product sense (ie: keep what the publisher wrote as much as possible), but it also makes the code easier and faster.
There was a problem hiding this comment.
Actually I rewrote the loop to make it easier to read and O(n), thanks Enrique.
| await story.layoutCallback(); | ||
|
|
||
| expect(story.storeService_.get(StateProperty.PAGE_IDS)).to.deep.equal([ | ||
| 'cover', |
There was a problem hiding this comment.
It's due to the condition set in the while ((index = pageIds.indexOf(pageIds[i], i + 1)) !== -1) { of line 531 in amp-story.js.
the index will take the value of the first duplicate it finds after the current i element. Thus, the current element remains unchanged.
Perhaps there is a way to make that loop a bit easier to read.
Deduplicates
amp-story-pageids and triggers a user error, so the story doesn't fail on load.Having duplicated IDs completely fails navigation or the progress bar.
becomes