Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions all.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ import './tabs/primary-tab.js'
import './tabs/secondary-tab.js'
import './tabs/tabs.js'
import './text/text-field.js'
import './carousel/carousel.js'
import './carousel/carousel-item.js'
Comment on lines +46 to +47
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).

// go/keep-sorted end
// LINT.ThenChange(:exports)
// LINT.IfChange(exports)
Expand Down Expand Up @@ -74,3 +76,5 @@ export * from './switch/switch.js'
export * from './tabs/tab.js'
export * from './tabs/tabs.js'
export * from './text/text-field.js'
export * from './carousel/carousel.js'
export * from './carousel/carousel-item.js'
Comment on lines +79 to +80
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).

32 changes: 32 additions & 0 deletions carousel/carousel-item.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import { LitElement, html, css } from 'lit';

/**
* A Material Design carousel item component.
*/
export class CarouselItem extends LitElement {
static get styles() {
return css`
:host {
display: block;
scroll-snap-align: start;
flex: 0 0 auto;
}
`;
}

constructor() {
super();
this.internals = this.attachInternals();
this.internals.role = 'listitem';
}

render() {
return html`
<div>
<slot></slot>
</div>
`;
Comment on lines +24 to +28
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>
    `;
  }

}
}

customElements.define('md-carousel-item', CarouselItem);
50 changes: 50 additions & 0 deletions carousel/carousel.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import { LitElement, html, css } from 'lit';

/**
* A Material Design carousel component.
*/
export class Carousel extends LitElement {
static get styles() {
return css`
: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;
}
Comment on lines +9 to +30
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;
      }

`;
}

constructor() {
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.

}

render() {
return html`
<div class="container">
<slot></slot>
</div>
`;
}
}

customElements.define('md-carousel', Carousel);
4 changes: 4 additions & 0 deletions common.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import './select/select-option.js'
import './tabs/primary-tab.js'
import './tabs/tabs.js'
import './text/text-field.js'
import './carousel/carousel.js'
import './carousel/carousel-item.js'

export * from './buttons/button.js'
export * from './checkbox/checkbox.js'
Expand All @@ -45,3 +47,5 @@ export * from './select/select-option.js'
export * from './tabs/tab.js'
export * from './tabs/tabs.js'
export * from './text/text-field.js'
export * from './carousel/carousel.js'
export * from './carousel/carousel-item.js'
5 changes: 1 addition & 4 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.