Skip to content

feat(carousel): add carousel component#2129

Draft
chintankavathia wants to merge 1 commit into
mainfrom
feat/carousel
Draft

feat(carousel): add carousel component#2129
chintankavathia wants to merge 1 commit into
mainfrom
feat/carousel

Conversation

@chintankavathia
Copy link
Copy Markdown
Member

@chintankavathia chintankavathia commented Jun 5, 2026

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 SiCarouselComponent and updates the CriterionDefinition interface in the filtered-search module. The review feedback identifies a style guide violation regarding the use of effect() for state propagation, suggests removing non-idiomatic 'this.' references in Angular templates, and highlights several UX writing guideline violations in the carousel example component, specifically regarding capitalization, vocabulary, and the use of abbreviations.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +112 to +214
readonly currentPage = signal(0);
readonly totalPages = computed(() => Math.ceil(this.items().length / this.slidesPerPage()));
readonly pages = computed(() => Array.from({ length: this.totalPages() }, (_, i) => i));
protected readonly pageGroups = computed(() => {
const groupSize = Math.ceil(this.totalPages() / MAX_PAGES);
return Array.from({ length: groupSize }, (_, i) =>
this.pages().slice(i * MAX_PAGES, (i + 1) * MAX_PAGES)
);
});
protected readonly activeGroup = computed(() => Math.floor(this.currentPage() / MAX_PAGES));
/**
* Track position used for the transform; equals `totalPages()` only briefly while the
* forward-wrap illusion is running. During that window the first-page items are
* translated to appear after the last page, so the track can slide forward by one
* page before snapping back to 0.
*/
protected readonly displayPage = signal(0);
protected readonly noTransition = signal(false);

protected readonly trackWidth = computed(() => {
return (
'calc(' +
this.totalPages() +
' * 100cqi + ' +
(this.totalPages() - 1) +
' * var(--si-carousel-gap))'
);
});

protected readonly gridTemplateColumns = computed(() => {
return 'repeat(' + this.slidesPerPage() * this.totalPages() + ', 1fr)';
});

protected readonly transform = computed(() => {
return `translateX(calc(${-this.displayPage()} * (100cqi + var(--si-carousel-gap))))`;
});

private intervalId: ReturnType<typeof setInterval> | null = null;
constructor() {
effect(onCleanup => {
this.scheduleAutoPlay();
onCleanup(() => clearInterval(this.intervalId!));
});

effect(() => {
// Reset to the first page if the current page becomes out of range due to items being added/removed.
if (this.currentPage() >= this.totalPages()) {
this.goToPage(0);
}
});
}

goToPage(page: number): void {
this.clearWrap();
this.noTransition.set(false);
this.currentPage.set(page);
this.displayPage.set(page);
this.scheduleAutoPlay();
}

next(): void {
const total = this.totalPages();
if (total <= 1) {
return;
}
this.noTransition.set(false);
const current = this.currentPage();
if (current === total - 1) {
// Shift first-page items so they visually appear after the last page, then
// animate the track forward by one page. The snap back happens in
// onTransitionEnd.
this.applyWrapForward();
this.displayPage.set(total);
this.currentPage.set(0);
} else {
const next = current + 1;
this.displayPage.set(next);
this.currentPage.set(next);
}
this.scheduleAutoPlay();
}

previous(): void {
const total = this.totalPages();
if (total <= 1) {
return;
}
this.noTransition.set(false);
const current = this.currentPage();
if (current === 0) {
// Shift last-page items so they visually appear before the first page, then
// animate the track backward by one page. The snap forward happens in
// onTransitionEnd.
this.applyWrapBackward();
this.displayPage.set(-1);
this.currentPage.set(total - 1);
} else {
const prev = current - 1;
this.displayPage.set(prev);
this.currentPage.set(prev);
}
this.scheduleAutoPlay();
}
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 component uses effect() to reset currentPage when totalPages changes. This is a direct violation of the repository style guide, which states that effect() should not be used for propagation of state changes as it can lead to ExpressionChangedAfterItHasBeenChecked errors or unnecessary change detection cycles.

Instead, we can introduce a private _currentPage writable signal and make currentPage a read-only computed signal that clamps the value of _currentPage to be within the bounds of totalPages. This also improves encapsulation by preventing external modification of currentPage directly.

  /** @defaultValue 0 */
  private readonly _currentPage = signal(0);
  readonly currentPage = computed(() => {
    const total = this.totalPages();
    const current = this._currentPage();
    return total > 0 && current >= total ? 0 : current;
  });
  readonly totalPages = computed(() => Math.ceil(this.items().length / this.slidesPerPage()));
  readonly pages = computed(() => Array.from({ length: this.totalPages() }, (_, i) => i));
  protected readonly pageGroups = computed(() => {
    const groupSize = Math.ceil(this.totalPages() / MAX_PAGES);
    return Array.from({ length: groupSize }, (_, i) =>
      this.pages().slice(i * MAX_PAGES, (i + 1) * MAX_PAGES)
    );
  });
  protected readonly activeGroup = computed(() => Math.floor(this.currentPage() / MAX_PAGES));
  /**
   * Track position used for the transform; equals `totalPages()` only briefly while the
   * forward-wrap illusion is running. During that window the first-page items are
   * translated to appear after the last page, so the track can slide forward by one
   * page before snapping back to 0.
   */
  protected readonly displayPage = signal(0);
  protected readonly noTransition = signal(false);

  protected readonly trackWidth = computed(() => {
    return (
      'calc(' +
      this.totalPages() +
      ' * 100cqi + ' +
      (this.totalPages() - 1) +
      ' * var(--si-carousel-gap))'
    );
  });

  protected readonly gridTemplateColumns = computed(() => {
    return 'repeat(' + this.slidesPerPage() * this.totalPages() + ', 1fr)';
  });

  protected readonly transform = computed(() => {
    return 'translateX(calc(' + -this.displayPage() + ' * (100cqi + var(--si-carousel-gap))))';
  });

  private intervalId: ReturnType<typeof setInterval> | null = null;
  constructor() {
    effect(onCleanup => {
      this.scheduleAutoPlay();
      onCleanup(() => clearInterval(this.intervalId!));
    });
  }

  goToPage(page: number): void {
    this.clearWrap();
    this.noTransition.set(false);
    this._currentPage.set(page);
    this.displayPage.set(page);
    this.scheduleAutoPlay();
  }

  next(): void {
    const total = this.totalPages();
    if (total <= 1) {
      return;
    }
    this.noTransition.set(false);
    const current = this.currentPage();
    if (current === total - 1) {
      // Shift first-page items so they visually appear after the last page, then
      // animate the track forward by one page. The snap back happens in
      // onTransitionEnd.
      this.applyWrapForward();
      this.displayPage.set(total);
      this._currentPage.set(0);
    } else {
      const next = current + 1;
      this.displayPage.set(next);
      this._currentPage.set(next);
    }
    this.scheduleAutoPlay();
  }

  previous(): void {
    const total = this.totalPages();
    if (total <= 1) {
      return;
    }
    this.noTransition.set(false);
    const current = this.currentPage();
    if (current === 0) {
      // Shift last-page items so they visually appear before the first page, then
      // animate the track backward by one page. The snap forward happens in
      // onTransitionEnd.
      this.applyWrapBackward();
      this.displayPage.set(-1);
      this._currentPage.set(total - 1);
    } else {
      const prev = current - 1;
      this.displayPage.set(prev);
      this._currentPage.set(prev);
    }
    this.scheduleAutoPlay();
  }
References
  1. Do NOT use effect() for propagation of state changes — this leads to ExpressionChangedAfterItHasBeenChecked errors, infinite circular updates, or unnecessary change detection cycles. Use computed() instead to model state that depends on other state. (link)

(click)="previous()"
></button>
<div class="d-flex align-items-center gap-2">
@for (page of pageGroups()[this.activeGroup()]; track page) {
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

In Angular templates, referencing component properties with this. is non-idiomatic and unnecessary. It should be removed.

Suggested change
@for (page of pageGroups()[this.activeGroup()]; track page) {
@for (page of pageGroups()[activeGroup()]; track page) {

[withoutPageControls]="withoutPageControls()"
>
<ng-template #totalUsers>
<si-card heading="Total Users" subHeading="Last 30 days">
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 card heading "Total Users" violates the UX writing guidelines regarding capitalization ("Capitalize only the first letter of the first word in titles..."). Also, "Last 30 days" violates the time-based vocabulary guideline ("Use 'latest' (most recent, more may follow) not 'last'").

      <si-card heading="Total users" subHeading="Latest 30 days">
References
  1. Capitalize only the first letter of the first word in titles, tooltips, menu items, list items, and buttons. (link)
  2. Use 'latest' (most recent, more may follow) not 'last' (implies nothing else will follow). (link)

Comment on lines +66 to +67
heading="Premium Product"
subHeading="Advanced Analytics Suite"
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 card heading "Premium Product" and subHeading "Advanced Analytics Suite" violate the UX writing guidelines regarding capitalization ("Capitalize only the first letter of the first word in titles...").

        heading="Premium product"
        subHeading="Advanced analytics suite"
References
  1. Capitalize only the first letter of the first word in titles, tooltips, menu items, list items, and buttons. (link)

</si-card>
</ng-template>
<ng-template #accentInfoCard>
<si-card class="accent-info" heading="Info accent">
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 card heading "Info accent" uses the abbreviation "Info", which violates the UX writing guidelines ("Avoid abbreviations (info, incl, excl) and acronyms").

      <si-card class="accent-info" heading="Information accent">
References
  1. Avoid abbreviations (info, incl, excl) and acronyms. (link)

</si-card>
</ng-template>
<ng-template #accentWarningCard>
<si-card class="accent-warning" heading="Warning accent">
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 card heading "Warning accent" uses the word "Warning", which violates the UX writing guidelines ("Avoid using the word 'error' or 'warning' — these are superfluous when design elements (icons, color) convey severity").

      <si-card class="accent-warning" heading="Attention accent">
References
  1. Avoid using the word 'error' or 'warning' — these are superfluous when design elements (icons, color) convey severity. (link)

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