feat: Implement Material 3 image carousel component#113
Conversation
Added `md-carousel` and `md-carousel-item` components based on Material 3 specifications. The implementation uses CSS Scroll Snap for horizontal scrolling and modern CSS Scroll-driven Animations (`animation-timeline: view(inline)`) to achieve the dynamic item resizing (large in the center, shrinking at the edges) characteristic of the M3 multi-browse design, ensuring high performance without heavy JavaScript computations. Components have correct ARIA roles and export configurations. 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 Carousel component, consisting of Carousel and CarouselItem custom elements, and integrates them into the project's main entry points. The implementation leverages Lit and CSS Scroll-driven Animations for dynamic item resizing. Review feedback focuses on improving accessibility by removing a hardcoded aria-label, ensuring cross-browser compatibility for animations by adding a duration to the shorthand property, and unifying CSS variable names for better maintainability.
|
|
||
| render() { | ||
| return html` | ||
| <div class="carousel" aria-label="carousel"> |
There was a problem hiding this comment.
The aria-label="carousel" is a hardcoded string, which is problematic for internationalization. Additionally, aria-label should generally not be used on a div without a semantic role (like region or list), as it may be ignored by assistive technologies. Since the host element already has role="list", any descriptive label should be provided by the consumer on the host element itself.
| <div class="carousel" aria-label="carousel"> | |
| <div class="carousel"> |
| scroll-snap-align: start; | ||
|
|
||
| /* M3 dynamic item sizing using CSS Scroll-driven Animations */ | ||
| animation: material-carousel-item-resize linear both; |
There was a problem hiding this comment.
The animation shorthand is missing a duration. While animation-timeline is used to drive the progress, many browsers still require a non-zero duration (or the auto keyword) for the animation to be considered active and to function correctly across different implementations of the Scroll-driven Animations spec.
| animation: material-carousel-item-resize linear both; | |
| animation: material-carousel-item-resize auto linear both; |
| 15% { | ||
| width: var(--md-carousel-large-width, 240px); | ||
| } | ||
| 85% { | ||
| width: var(--md-carousel-large-width, 240px); | ||
| } |
There was a problem hiding this comment.
The variable name --md-carousel-large-width is inconsistent with --md-carousel-item-width used in carousel-item.js. Using a single variable for the item's standard width improves maintainability and ensures that customizations to the item size are correctly reflected in the carousel's dynamic resizing animation.
| 15% { | |
| width: var(--md-carousel-large-width, 240px); | |
| } | |
| 85% { | |
| width: var(--md-carousel-large-width, 240px); | |
| } | |
| 15% { | |
| width: var(--md-carousel-item-width, 240px); | |
| } | |
| 85% { | |
| width: var(--md-carousel-item-width, 240px); | |
| } |
1 participant
Implemented the Material 3 image carousel component with dynamic item resizing using modern CSS.
PR created automatically by Jules for task 15702534854147387189 started by @treeder