馃悰 amp-base-carousel: Update offset and index together #30963
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
micajuine-ho
force-pushed
the
carouselFix
branch
from
879f7c5
to
15148c3
Compare
|
|
||
| // Fake the scroll that ends short of the correct index. | ||
| // This is handled by scroll event listener. | ||
| carousel.touching_ = false; |
There was a problem hiding this comment.
Thank you for adding a comment here describing what the intent of these values are, nice touch!
| describe('resetScrollReferencePoint_', () => { | ||
| it('currentElementOffset_ & currentIndex_ should be set when it is a' + | ||
| ' programmatic scroll', async () => { | ||
| setStyle(element, 'width', '299.2px'); |
There was a problem hiding this comment.
Does this condition happen only when the element width is not floored?
There was a problem hiding this comment.
No, this is actually unnecessary. I'll simplify.
micajuine-ho
force-pushed
the
carouselFix
branch
2 times, most recently
from
372629c
to
e6fc9b3
Compare
micajuine-ho
force-pushed
the
carouselFix
branch
from
e6fc9b3
to
ba5d222
Compare
dmanek
approved these changes
Nov 3, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #30905
When the viewport is barely bigger than the carousel (on the scrolling axis), the scroll listener will not receive the final event that would set
currentElementOffset_andcurrentIndex_to0andn+1(instead it would be set at something like-0.998,n).Then, when
resetScrollReferencePoint_()is finally called, it will updatecurrentIndex_to the correct value, but leavecurrentElementOffset_to be referencing the old index value.amphtml/extensions/amp-base-carousel/0.1/carousel.js
Lines 1182 to 1187 in f479ea6
Then finally, when
restoreScrollStart_()is called,deltaOffsetwill not be 0, due toactualOffsetandcurrentElementOffset_referencing 2 different elements (causing another shift in the carousel).amphtml/extensions/amp-base-carousel/0.1/carousel.js
Line 1229 in f479ea6