馃悰 [amp-carousel 0.2] Set width for responsive slides #31564
Conversation
micajuine-ho
force-pushed
the
responsiveCarouselBug
branch
from
f4621de
to
1ffcc21
Compare
micajuine-ho
force-pushed
the
responsiveCarouselBug
branch
from
1ffcc21
to
ca58d63
Compare
micajuine-ho
force-pushed
the
responsiveCarouselBug
branch
from
ca58d63
to
1e49863
Compare
| */ | ||
|
|
||
| const TAG_NAME = 'amp-carousel'; | ||
| const SCROLLABLE_SLIDE = 'amp-scrollable-carousel-slide'; |
There was a problem hiding this comment.
I wonder if there is a pattern here we could adopt similar to CSS Modules, allowing the test helper to import the name from the CSS file.
No reason to change for this PR, but in the future it would help to prevent skew between names.
There was a problem hiding this comment.
Let me see if this is possible.
|
Do we need these image files?
Could the slides use a |
| export function getSlides(controller) { | ||
| return controller.findElements(`${TAG_NAME} .${SCROLLABLE_SLIDE}`); | ||
| } | ||
|
|
||
| export async function getSlide(controller, n) { | ||
| const slides = await getSlides(controller); | ||
| return slides[n]; | ||
| } | ||
|
|
||
| export function getNextArrow(controller) { | ||
| return controller.findElement(NEXT_ARROW_SELECTOR); | ||
| } | ||
|
|
||
| export function sleep(ms) { | ||
| return new Promise((res) => setTimeout(res, ms)); | ||
| } |
There was a problem hiding this comment.
Some of these appear to be taken from the helpers for amp-base-carousel tests. Can they be shared instead?
There was a problem hiding this comment.
They are (and same w/ the images), but I wasn't sure if cross extension dependencies (even if its just in tests) is good practice. WDYT?
Let's try this again....
Closes #31304
Without a set width on the slide element (for type=carousel), the spacer (that gets created by runtime for responsive elements) inside the slide will have a height and width set to 0x0.
When the carousel lays out its children, runtime stops layoutCallback() from being called because it has 0 height and width (because spacer is not correctly inflating the element).
The fix is to add width: 100% to the slides that are responsive (since this could affect a lot of pubs, let's be really restrictive on the CSS ruleset here).
This issue doesn't occur in
amp-base-carouseldue to the immediate parent of the slides (the scroll container) havingwidth:100%set. The same this is true ofamp-carousel type=slides-- immediate parent of the slides (the wrapper in this case) haswidth:100%set.