✨Implement amp-stream-gallery. #24388
Conversation
- Fix check for overlapping slide sometimes missing one slide (fix < to <=). - Pause layout internal to carousel implementation, rather than just pausing autoadvance.
- Fix bug with max-item-width - Fix inset custom arrows width collapsing to zero
- Fix bug where a scroll event inside carousel could be ignored if a previous programmatic scroll of 0px was attempted.
- Rework the workaround for flickering slides on iOS to toggle the change queuing on scroll, which is more robust to when the intersection observer may fire.
- Fix test's event name.
sparhami
changed the title
|
|
||
| // No scroll will happen, ,ake sure the `ignoreNextScroll_` flag is not | ||
| // set. | ||
| if (scrollPos == pos) { |
There was a problem hiding this comment.
It was my understanding that === is generally preferred over ==, but I'm not certain if that's an intentionally rejected practice in this repo for performance or other reasons.
There was a problem hiding this comment.
=== is better for performance, but I think the mixture comes from the fact that the Google JS style guide used to prefer == over === (except when you need to distinguish between null and undefined). It is now flipped, to prefer ===. I'll update the checks here to reflext that.
Most of the open source ecosystem prefers ===, only using == when necessary.
- Change equality checks to use `===` - Fix typo
danielrozenberg
left a comment
danielrozenberg
left a comment
There was a problem hiding this comment.
Approval for dep-check-config.js
- Refactor amp-base-carousel to more closely match amp-stream-gallery's code patterns. - Track the current offset in the slide as a percentage rather than a pixel offset, which will restore scroll position more accurately when resizing. - Use intersection observer for visibility tracking of child slides for amp-base-carousel. - Fix a couple of edge cases (`<=` vs `<`).
sparhami
requested review from
wassgha
and removed request for
aghassemi and
alanorozco
|
This cannot be updated without CLA issues, so I'll close in favor of #26710 |
6 participants
Implements amp-stream-gallery, which is a carousel for showing multiple items at once. This is useful for things like related products and articles.
This differs from amp-base-carousel in the following ways:
Fixes:
<=).
pausing auto advance.
For #20595