feat: implement Material 3 carousel component#115
Conversation
- Implemented `md-carousel` and `md-carousel-item` components using Lit. - Implemented Material 3 standard visual styling including CSS flexbox and `scroll-snap`. - Applied correct ARIA accessibility roles using `attachInternals()`. - Updated `all.js` and `common.js` to export these new components. - Added a demo in `demo/carousel.html` and linked it to the main `demo/index.html`. 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. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new Material 3 Carousel component, consisting of md-carousel and md-carousel-item elements, along with a demo page and integration into the main entry points. The review feedback focuses on improving the component's robustness and flexibility: specifically, ensuring correct scroll snapping by adding scroll-padding, enhancing accessibility for keyboard users by making the scrollable container focusable, and utilizing CSS custom properties for better themeability and customization of dimensions and styles.
| :host { | ||
| display: flex; | ||
| overflow-x: auto; | ||
| scroll-snap-type: x mandatory; | ||
| gap: 8px; | ||
| padding: 16px; | ||
| scrollbar-width: none; /* Firefox */ | ||
| -ms-overflow-style: none; /* IE/Edge */ | ||
| } |
There was a problem hiding this comment.
When using scroll-snap-type with container padding, it is important to set scroll-padding to match. This ensures that items snap correctly relative to the padding rather than the edge of the scroll container. Additionally, using CSS custom properties for spacing improves themeability and adherence to Material Design principles.
| :host { | |
| display: flex; | |
| overflow-x: auto; | |
| scroll-snap-type: x mandatory; | |
| gap: 8px; | |
| padding: 16px; | |
| scrollbar-width: none; /* Firefox */ | |
| -ms-overflow-style: none; /* IE/Edge */ | |
| } | |
| :host { | |
| display: flex; | |
| overflow-x: auto; | |
| scroll-snap-type: x mandatory; | |
| scroll-padding: var(--md-carousel-container-padding, 16px); | |
| gap: var(--md-carousel-container-gap, 8px); | |
| padding: var(--md-carousel-container-padding, 16px); | |
| scrollbar-width: none; /* Firefox */ | |
| -ms-overflow-style: none; /* IE/Edge */ | |
| } |
| constructor() { | ||
| super(); | ||
| this.internals = this.attachInternals(); | ||
| this.internals.role = 'list'; | ||
| } |
There was a problem hiding this comment.
The carousel container should be focusable to support keyboard navigation. Since scrollbars are hidden (lines 18-24), keyboard users rely on arrow keys to scroll the content, which requires the scrollable element to be focusable via tabindex.
| constructor() { | |
| super(); | |
| this.internals = this.attachInternals(); | |
| this.internals.role = 'list'; | |
| } | |
| constructor() { | |
| super(); | |
| this.internals = this.attachInternals(); | |
| this.internals.role = 'list'; | |
| this.tabIndex = 0; | |
| } |
| :host { | ||
| display: block; | ||
| border-radius: 16px; | ||
| overflow: hidden; | ||
| /* Default aspect ratio, can be overridden */ | ||
| aspect-ratio: 1; | ||
| width: 200px; /* Default width, can be overridden */ | ||
| scroll-snap-align: start; | ||
| flex: 0 0 auto; | ||
| } |
There was a problem hiding this comment.
Consider using CSS custom properties for dimensions and styles like border-radius, aspect-ratio, and width. This allows consumers to easily customize the carousel items to fit different Material 3 carousel types (e.g., hero, multi-browse, uncontained) without needing to override the entire component's styles.
| :host { | |
| display: block; | |
| border-radius: 16px; | |
| overflow: hidden; | |
| /* Default aspect ratio, can be overridden */ | |
| aspect-ratio: 1; | |
| width: 200px; /* Default width, can be overridden */ | |
| scroll-snap-align: start; | |
| flex: 0 0 auto; | |
| } | |
| :host { | |
| display: block; | |
| border-radius: var(--md-carousel-item-container-shape, 16px); | |
| overflow: hidden; | |
| /* Default aspect ratio, can be overridden */ | |
| aspect-ratio: var(--md-carousel-item-aspect-ratio, 1); | |
| width: var(--md-carousel-item-width, 200px); /* Default width, can be overridden */ | |
| scroll-snap-align: start; | |
| flex: 0 0 auto; | |
| } |
1 participant
Implemented a Material 3 image carousel component using Lit, including CSS scroll-snapping and a demo page.
PR created automatically by Jules for task 5505580702602107449 started by @treeder