✨ Carousel: InlineGallery and Pagination components #29674
Conversation
| return ( | ||
| <> | ||
| <WithStyles> | ||
| <InlineGallery style={{width, position: 'relative'}}> |
There was a problem hiding this comment.
IMHO, InlineGallery should be inherently relative. Should we try to use ContainWrapper on it?
There was a problem hiding this comment.
Ideally InlineGallery is more like layout=container -- its height is exactly the sum of its children's heights. IMO it is a bit clunky to have to specify this manually -- if I size my BaseCarousel and don't size Pagination (with default height of 20px) this should just work.
There was a problem hiding this comment.
Definitely agree with this. But ContainWrapper doesn't have to be size=true, which should work here - it'd rely on its children to provide the size.
There was a problem hiding this comment.
The latest changes use ContainWrapper, but still need to resolve edge cases when there are multiple Pagination components and a subset of them are inset.
| import {createContext} from '../../../src/preact'; | ||
|
|
||
| const CarouselContext = createContext( | ||
| /** @type {BaseCarouselDef.ContextProps} */ ({}) |
There was a problem hiding this comment.
Hmm. But now we do need to provide setSlideCount and setCurrentSlide here - right? They'll just be empty functions, but then (a) we don't need to test for them everywhere, and (b) it self-documents.
There was a problem hiding this comment.
Happy to put setSlideCount, however setCurrentSlide should fallback to the one given by useState if the CarouselContext is not used. One option could be to use useState in carousel-context as the default value instead, but I believe this will have an issue with being called at the top level. WDYT?
There was a problem hiding this comment.
I see. I didn't think about that. Ok. In that case, i'm all good with the undefined function fields and checks. Sorry for the confusion.
There was a problem hiding this comment.
We can still define setSlideCount, just not setCurrentSlide! I did this in ae93511.
There was a problem hiding this comment.
Sounds good. Let's go with that.
* Add storybook sample * Prototype * Add Storybook for looping * Update styles * Use context provider * Update unit tests * Use ContainWrapper * Pull context into a separate file * Update slide count in useEffect * Define default `setSlideCount`
Alternative approach to #29545