feat: Add Material 3 Carousel component#112
Conversation
Implements the `md-carousel` and `md-carousel-item` web components using Lit and CSS scroll snap according to Material Design 3 guidelines. - `md-carousel` provides the horizontally scrollable container with snap points. - `md-carousel-item` represents individual items within the carousel. - Accessibility is handled natively on the host elements via `ElementInternals` (`role="list"` and `role="listitem"`). - Components are exposed via bundle entry points `all.js` and `common.js`. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Code Review
This pull request introduces a new Material Design carousel component, including Carousel and CarouselItem elements, and updates project exports. Feedback includes a critical fix for scroll snapping by moving CSS properties to the container, a request to maintain alphabetical sorting for imports and exports to comply with existing directives, the removal of a redundant wrapper in the carousel item, and a recommendation to make the carousel's ARIA label configurable for better accessibility and internationalization.
| :host { | ||
| display: block; | ||
| width: 100%; | ||
| overflow-x: auto; | ||
| overscroll-behavior-x: contain; | ||
| scroll-snap-type: x mandatory; | ||
| scrollbar-width: none; /* Firefox */ | ||
| } | ||
|
|
||
| :host::-webkit-scrollbar { | ||
| display: none; /* Safari and Chrome */ | ||
| } | ||
|
|
||
| .container { | ||
| display: flex; | ||
| gap: 8px; /* Default gap */ | ||
| } | ||
|
|
||
| ::slotted(*) { | ||
| scroll-snap-align: start; | ||
| flex: 0 0 auto; | ||
| } |
There was a problem hiding this comment.
The current CSS structure prevents scroll snapping from working correctly for slotted items. In CSS, scroll-snap-type must be defined on the scroll container, and scroll-snap-align must be defined on its direct children. Currently, scroll-snap-type is on the :host, but the items are nested inside .container, making .container the only direct child of the scroll container.
Moving the overflow and snapping properties to the .container element will fix this issue.
:host {
display: block;
width: 100%;
}
.container {
display: flex;
gap: 8px; /* Default gap */
overflow-x: auto;
overscroll-behavior-x: contain;
scroll-snap-type: x mandatory;
scrollbar-width: none; /* Firefox */
}
.container::-webkit-scrollbar {
display: none; /* Safari and Chrome */
}
::slotted(*) {
scroll-snap-align: start;
flex: 0 0 auto;
}| import './carousel/carousel.js' | ||
| import './carousel/carousel-item.js' |
| export * from './carousel/carousel.js' | ||
| export * from './carousel/carousel-item.js' |
| return html` | ||
| <div> | ||
| <slot></slot> | ||
| </div> | ||
| `; |
| super(); | ||
| this.internals = this.attachInternals(); | ||
| this.internals.role = 'list'; | ||
| this.internals.ariaLabel = 'Carousel'; |
There was a problem hiding this comment.
This PR adds a new
md-carouselcomponent along withmd-carousel-item. The carousel is built using modern CSSscroll-snapfor smooth and native-feeling horizontal scrolling. It properly sets uplistandlistitemARIA roles usingElementInternalson the host elements, avoiding issues with crossing shadow DOM boundaries. The components are also exported fromall.jsandcommon.js.PR created automatically by Jules for task 6654690335110854632 started by @treeder