feat: implement Material 3 Carousel components#111
Conversation
Added Material Design 3 image carousel components using modern web standards (LitElement, shadow DOM, flexbox, CSS scroll snap). Includes demo updates and comprehensive component documentation. 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 3 carousel component, including the md-carousel and md-carousel-item elements, along with documentation and a demo. Feedback focuses on enhancing accessibility and component robustness. Key recommendations include moving the listitem role to the host element for better screen reader support, internationalizing the default ARIA label, and ensuring scroll-padding matches container padding for accurate scroll snapping. Additionally, the reviewer suggests reconsidering the hidden scrollbar to improve usability and removing development notes from the production code.
| export class CarouselItem extends LitElement { | ||
| render() { | ||
| return html` | ||
| <div class="item-container" role="listitem"> | ||
| <slot></slot> | ||
| </div> | ||
| ` | ||
| } |
There was a problem hiding this comment.
The role="listitem" should be applied to the host element rather than an internal div. This ensures that the component is correctly identified as a list item by assistive technologies when placed inside a role="list" container (like the one in md-carousel).
export class CarouselItem extends LitElement {
connectedCallback() {
super.connectedCallback()
if (!this.hasAttribute('role')) {
this.setAttribute('role', 'listitem')
}
}
render() {
return html`
<div class="item-container">
<slot></slot>
</div>
`
}|
|
||
| constructor() { | ||
| super() | ||
| this.ariaLabel = 'Carousel' |
| :host { | ||
| display: block; | ||
| /* Carousel styling based on Material Design 3 */ | ||
| /* Note: specific width/height may need to be configurable */ |
| scrollbar-width: none; /* Firefox */ | ||
| -ms-overflow-style: none; /* IE and Edge */ | ||
|
|
||
| padding: var(--md-carousel-padding, 0); |
There was a problem hiding this comment.
When using scroll-snap-type, it is important to set scroll-padding to match the container's padding. This ensures that items snap correctly relative to the visible area of the carousel, preventing them from being partially obscured by the padding.
| padding: var(--md-carousel-padding, 0); | |
| padding: var(--md-carousel-padding, 0); | |
| scroll-padding: var(--md-carousel-padding, 0); |
| scroll-snap-type: x mandatory; | ||
| overscroll-behavior-x: contain; | ||
|
|
||
| /* Hide scrollbar for cleaner look, but keep functionality */ |
There was a problem hiding this comment.
Hiding the scrollbar can negatively impact accessibility and usability, as it removes a visual cue for the amount of content and a means of navigation for some users. Consider allowing the scrollbar to be visible or providing alternative navigation indicators (like dots or arrows) as per Material Design 3 guidelines.
Implemented Material Design 3 carousel components.
Includes
md-carouselandmd-carousel-itemwritten as Lit elements leveraging modern CSS capabilities like flexbox and scroll snapping to achieve native-feeling interactions. Tested visually via Playwright.Components:
md-carousel: The horizontal scroll container (role="list").md-carousel-item: The individual snap targets (role="listitem").Added demo showcasing usage with
md-cardand documented customizable CSS variables in a newREADME.md.PR created automatically by Jules for task 8823521121697351188 started by @treeder