Skip to content

feat: Add Material 3 Carousel component#112

Draft
treeder wants to merge 1 commit into
mainfrom
feat-carousel-component-6654690335110854632
Draft

feat: Add Material 3 Carousel component#112
treeder wants to merge 1 commit into
mainfrom
feat-carousel-component-6654690335110854632

Conversation

@treeder
Copy link
Copy Markdown
Member

@treeder treeder commented May 12, 2026

This PR adds a new md-carousel component along with md-carousel-item. The carousel is built using modern CSS scroll-snap for smooth and native-feeling horizontal scrolling. It properly sets up list and listitem ARIA roles using ElementInternals on the host elements, avoiding issues with crossing shadow DOM boundaries. The components are also exported from all.js and common.js.


PR created automatically by Jules for task 6654690335110854632 started by @treeder

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>
@google-labs-jules
Copy link
Copy Markdown
Contributor

👋 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 @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread carousel/carousel.js
Comment on lines +9 to +30
: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;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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;
      }

Comment thread all.js
Comment on lines +46 to +47
import './carousel/carousel.js'
import './carousel/carousel-item.js'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

These imports are placed at the end of a block marked with go/keep-sorted, which violates the alphabetical sorting requirement. They should be moved further up in the file (e.g., after the buttons imports).

Comment thread all.js
Comment on lines +79 to +80
export * from './carousel/carousel.js'
export * from './carousel/carousel-item.js'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

These exports are placed at the end of a go/keep-sorted block, violating the alphabetical sort order. They should be moved to their correct alphabetical position (e.g., after the buttons exports).

Comment thread carousel/carousel-item.js
Comment on lines +24 to +28
return html`
<div>
<slot></slot>
</div>
`;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The div wrapper around the slot is redundant as it provides no additional styling or layout benefits. Removing it simplifies the shadow DOM structure.

  render() {
    return html`
      <slot></slot>
    `;
  }

Comment thread carousel/carousel.js
super();
this.internals = this.attachInternals();
this.internals.role = 'list';
this.internals.ariaLabel = 'Carousel';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Hardcoding the ariaLabel to "Carousel" prevents internationalization and doesn't allow developers to provide a more descriptive label for the carousel's content (e.g., "Featured Products"). Consider making this a property or allowing it to be set via an attribute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant