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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
馃悰 [amp-carousel 0.1] Fix race for setting slideWidth_ #32866
Conversation
@@ -721,7 +721,8 @@ export class AmpSlideScroll extends BaseSlides { | |||
); | |||
return false; | |||
} | |||
showIndexArr.forEach((showIndex, loopIndex) => { | |||
|
|||
showIndexArr.sort().forEach((showIndex, loopIndex) => { |
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.
super nit: I prefer .sort()
on its own line instead of chained with forEach. that is because it actually modified the showIndexArr
in addition to returning it.
i.e.
showIndexArry.sort();
showIndexArr.forEach(...)
@@ -907,6 +907,19 @@ describes.realWin( | |||
expect(ampSlideScroll.hasAttribute('loop')).to.be.false; | |||
}); | |||
|
|||
it.only('sets the correct scrollLeft for looping carousel', async () => { |
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.
rm .only :P
style.order
for looping slides25a341d
to
141a1b4
Compare
Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>
* init * fix race * Update extensions/amp-carousel/0.1/slidescroll.js Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com> Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com> (cherry picked from commit 25bb462)
* init * fix race * Update extensions/amp-carousel/0.1/slidescroll.js Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com> Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com> (cherry picked from commit 25bb462)
Hello. This issue persists in amp-carousel 0.2. |
Closes #32920
There's a race between setting the
slidesWidth_
viaattachedCallback
and using theslidesWidth_
inlayoutCallback
(expected to be non-zero, initially, ingetScrollLeftForIndex_()
).Let's set a flag to see if it's been set and manually set if it hasn't been yet.