Chore/update ng 20#590
Conversation
🦋 Changeset detectedLatest commit: 9f5395d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Important Review skippedToo many files! This PR contains 237 files, which is 87 over the limit of 150. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (8)
📒 Files selected for processing (237)
You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR performs major framework and build tooling upgrades: Angular 19→21, Storybook 8→10, Yarn 1.22→4.0.1. Sass modules migration converts all Changes
Sequence Diagram(s)The changes are too heterogeneous (Sass modules, Angular API deprecations, Storybook config, version upgrades across unrelated systems) to form a single coherent sequence diagram. No single feature or flow dominates; instead, this represents parallel, independent migrations across build tooling, template syntax, and module systems that do not meaningfully interact as sequential operations. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Justification: Extremely large scope with multiple high-complexity, heterogeneous changes requiring separate reasoning: (1) comprehensive Sass module refactoring affecting 100+ component stylesheets; (2) Angular structural directive replacement across 50+ templates with control flow semantics that vary per template; (3) ESLint flat-config migration with plugin integration; (4) major dependency upgrades (Angular 19→21, Storybook 8→10, Yarn 1.22→4.0.1) affecting build/runtime; (5) ComponentFactoryResolver removal from CDK integrations; (6) build script refactoring for new Sass tooling. Each category demands distinct domain knowledge; patterns are inconsistent across files. High risk of subtle breakage in templating or styling due to scope/precedence changes in module system and control flow syntax changes. Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/select/validators.ts (1)
80-84: Return null (not undefined) and don’t treat 0/'' as “no value”validate should return ValidationErrors | null. The current falsy check incorrectly skips validation for 0/'' values and returns undefined.
- validate(control: AbstractControl): ValidationErrors { - if (!this.selectRef.contentOptions || !control.value) { - return; - } + validate(control: AbstractControl): ValidationErrors | null { + if (!this.selectRef.contentOptions || control.value === null || control.value === undefined) { + return null; + }scripts/copy-resources.js (1)
1-50: Adjust CDK overlay import removal to match actual import pathWe’ve confirmed:
- Your project’s
.yarnrc.ymlsetsnodeLinker: node-modules, so PnP is not in use.- In
src/theme/style.scss, the overlay stylesheet is imported as@use '../../node_modules/@angular/cdk/overlay-prebuilt' as *;However, the replace regex in
scripts/copy-resources.jsonly strips imports of the form/@use 'node_modules\/@angular\/cdk\/overlay-prebuilt';\n?/and therefore won’t match the actual relative import.
To fix this, update the regex to cover the relative path and optional
as *. For example:--- a/scripts/copy-resources.js +++ b/scripts/copy-resources.js @@ (inside try block) - const tempStyleContent = styleContent.replace(/@use 'node_modules\/@angular\/cdk\/overlay-prebuilt';\n?/, ''); + // Remove any CDK overlay-prebuilt import, including relative node_modules paths + const importRegex = + /@use\s+['"](?:\.\.\/)+node_modules\/@angular\/cdk\/overlay-prebuilt['"](?:\s+as\s+\*)?;\n?/; + const tempStyleContent = styleContent.replace(importRegex, '');This ensures the script correctly removes the
@use '../../node_modules/@angular/cdk/overlay-prebuilt' as *;line before compiling.src/date-picker/calendar/header/component.ts (1)
151-163: Decade navigation step is off by a factor of 10The Decade case in src/date-picker/calendar/header/component.ts (around lines 159–163) currently shifts the anchor by
valueyears, not decades. To move the view by full decades, multiply by 10:case DateNavRange.Decade: { - anchor = this.anchor.add(value, YEAR); + anchor = this.anchor.add(value * 10, YEAR); break; }Please update this case so that each “next”/“previous” decade arrow moves the calendar by ten years rather than one.
src/date-picker/trigger/trigger.style.scss (1)
1-112: Prefix all theme helper calls with thevar.namespace
There are numerous remaining invocations ofuse-rgb,use-var,use-text-color, anduse-rgbawithout the requiredvar.alias. Please update each tovar.use-rgb,var.use-var,var.use-text-color, orvar.use-rgba.Files needing refactoring (examples of occurrences in parentheses):
- src/tree-select/tree-node.component.scss (background-color: use-rgb(p-6) at line 34)
- src/tag/check-tag/check-tag.component.scss (color: use-rgb(n-1) at line 18; border-radius: calc(#{use-var(inline-height-s)} / 2) at line 25)
- src/tag/tag.component.scss (multiple uses of use-rgb/use-var throughout, e.g. color: use-rgb($color) at line 6; border-radius: use-var(border-radius-m) at line 39)
- src/icon/icon.component.scss (margin-left/right: use-var(spacing-s) at lines 29, 33, 42)
- src/input/input.component.scss (border: 1px solid use-rgb(n-7) at line 11; color: use-text-color(main) at line 14; font-weight: use-var(font-weight-normal) at line 17)
- src/date-picker/trigger/trigger.style.scss (border-color: use-rgb(primary) at line 55)
- src/checkbox/checkbox.component.scss (margin-right: use-var(spacing-xxxl) at line 8; background-color: use-rgb(n-8) at line 35; color: use-text-color(disabled) at line 65)
- src/button/button.component.scss (border-radius: use-var(border-radius-m) at line 134; font-weight: use-var(font-weight-normal) at line 140)
After renaming, verify no un-prefixed usages remain (e.g., via
rg -P '(?<!var\.)use-(rgb|var|text-color|rgba)\(').src/button/button.component.scss (3)
125-142: Fix missing namespace on use-var calls (compile error).use-var must be prefixed after @use; these lines will fail at compile time.
Apply:
- border-radius: use-var(border-radius-m); + border-radius: var.use-var(border-radius-m); ... - font-weight: use-var(font-weight-normal); + font-weight: var.use-var(font-weight-normal);
194-204: Fix missing namespace on use-rgb (compile error).Same namespace issue within text button.
- color: use-rgb(primary); + color: var.use-rgb(primary); ... - @include text-button-sizes; + @include text-button-sizes;
238-246: Use transparent instead of none for background-color.background-color: none is invalid; use transparent.
- background-color: none; + background-color: transparent;
🧹 Nitpick comments (75)
src/select/validators.ts (2)
16-29: Lint suppression removal may surface no-extraneous-class on AuiSelectValidatorsIf the flat ESLint config still enforces @typescript-eslint/no-extraneous-class, this static-only class will be flagged. If CI complains, consider replacing the class with a const object to keep the same callsite API (AuiSelectValidators.includes) without changing runtime behavior.
-export class AuiSelectValidators { - static includes<T>( - options: T[], - trackFn: TrackFn<T> = val => val, - ): ValidatorFn { - return (control: AbstractControl) => - options.some(option => trackFn(option) === trackFn(control.value)) - ? null - : { - includes: control.value, - }; - } -} +export const AuiSelectValidators = { + includes<T>( + options: T[], + trackFn: TrackFn<T> = (val: T) => val, + ): ValidatorFn { + return (control: AbstractControl) => + options.some(option => trackFn(option) === trackFn(control.value)) + ? null + : { includes: control.value }; + }, +} as const;
66-74: Avoid subscription leaks with takeUntilDestroyedAdd Angular’s takeUntilDestroyed for automatic teardown on directive destroy.
- this.selectRef.contentOptions.changes - .pipe(startWith(this.selectRef.contentOptions)) + this.selectRef.contentOptions.changes + .pipe( + startWith(this.selectRef.contentOptions), + // auto-cleanup on destroy + takeUntilDestroyed(this.destroyRef), + ) .subscribe(() => { - if (this.onValidatorChange) { - this.onValidatorChange(); - } + this.onValidatorChange?.(); });Additional changes outside this hunk:
- import { AfterContentInit, Directive, Input, forwardRef } from '@angular/core'; + import { AfterContentInit, Directive, Input, forwardRef, DestroyRef, inject } from '@angular/core'; + import { takeUntilDestroyed } from '@angular/core/rxjs-interop'; @@ export class IncludesDirective<T> implements Validator, AfterContentInit { @@ - constructor(private readonly selectRef: SelectComponent<T>) {} + private readonly destroyRef = inject(DestroyRef); + constructor(private readonly selectRef: SelectComponent<T>) {}src/dialog/dialog-content/dialog-content.component.ts (4)
1-3: Prefer canonical imports from @angular/cdk/scrolling.Avoid re-export paths; import CdkScrollable and ScrollDispatcher from the scrolling package directly. This aligns with current CDK docs and reduces risk across major upgrades. (v9.material.angular.io, app.unpkg.com)
Apply:
-import { CdkScrollable, ScrollDispatcher } from '@angular/cdk/overlay'; +import { CdkScrollable, ScrollDispatcher } from '@angular/cdk/scrolling';
24-32: Constructor lint suppression removal: confirm no-useless-constructor isn’t re-triggered.This ctor is needed to wire DI into the base CdkScrollable; it only calls super, but is not “useless.” If the new flat ESLint config flags it, either re-disable just for this line or we can adjust the rule in eslint config for Angular DI patterns.
If it re-flags, I can push either a targeted disable:
// eslint-disable-next-line @typescript-eslint/no-useless-constructoror a config tweak—your call.
25-31: Make Directionality injection explicitly optional.Mark dir as @optional to avoid hard DI requirement where bidi isn’t provided. Angular injects null when optional deps are missing. (angular.dev)
-import { ElementRef, NgZone, ViewEncapsulation } from '@angular/core'; +import { ElementRef, NgZone, Optional, ViewEncapsulation } from '@angular/core'; @@ - ngZone: NgZone, - dir?: Directionality, + ngZone: NgZone, + @Optional() dir?: Directionality,
17-19: Nit: preserveWhitespaces: false is redundant.False is the framework default; safe to drop for less noise. (angular.dev)
- preserveWhitespaces: false,.yarnrc.yml (1)
1-3: Registry mirror may cause availability/latency issues outside CN; consider not hardcoding.
nodeLinker: node-modulesis fine for Yarn 4. FornpmRegistryServer, hardcoding the npmmirror endpoint can slow or break installs for US/CI runners. Prefer using the default registry or an environment override (YARN_NPM_REGISTRY_SERVER) in region-specific CI.Apply to revert to the default registry:
nodeLinker: node-modules - -npmRegistryServer: "https://registry.npmmirror.com".storybook/preview.ts (1)
11-17: Prefer dark-mode constant and channel.on/off.Use
DARK_MODE_EVENT_NAMEfromstorybook-dark-modeinstead of the raw'DARK_MODE'string; subscribe withonfor consistency and future-proofing.+import { DARK_MODE_EVENT_NAME } from 'storybook-dark-mode'; … -const channel = addons.getChannel(); -channel.addListener('DARK_MODE', isDark => { +const channel = addons.getChannel(); +channel.on(DARK_MODE_EVENT_NAME, (isDark: boolean) => { document .querySelector('html') .setAttribute('aui-theme-mode', isDark ? 'dark' : 'light'); });Reference usage from the addon docs.
stories/back-top/back-top.mdx (1)
1-1: Confirm Storybook v9 import path for MDX blocksDouble-check that
@storybook/addon-docs/blocksis valid with your Storybook v9 setup; recent versions typically exposeCanvas/Metafrom@storybook/blocks. If needed, switch back:-import { Canvas, Meta } from '@storybook/addon-docs/blocks'; +import { Canvas, Meta } from '@storybook/blocks';.stylelintrc (1)
30-30: Rule disablement acknowledged; consider documenting rationaleDisabling
scss/selector-no-redundant-nesting-selectormakes sense with Sass modules and nesting. Add a short comment in the repo (e.g., in CONTRIBUTING.md) to explain why this rule is off to guide future lint rule changes.src/anchor/anchor.component.scss (2)
13-24: Anchor pseudo-elements should be scoped with a positioned container.Add position: relative to .aui-anchor so its absolutely positioned ::before/::after are anchored to the block itself, not an ancestor.
.aui-anchor { padding: 30px 16px; + position: relative; border-left: 1px solid var.use-rgb(border);
4-4: Consider replacing hard-coded 20px with a theme spacing token.For consistency with the rest of the module-based tokens, prefer a var.use-var(spacing-*) value if there’s an equivalent to 20px.
Also applies to: 38-39
scripts/copy-resources.js (2)
34-37: Potential PnP issue: loadPaths including 'node_modules'.Under Yarn PnP, 'node_modules' may not exist. If third-party Sass modules are needed, prefer a custom importer that resolves via require.resolve; otherwise, drop this path.
Example importer snippet (if needed):
const importer = { findFileUrl(url) { try { return new URL('file://' + require.resolve(url)); } catch { return null; } } }; // then pass: importers: [importer]
13-21: Optional: avoid gulp here and await completion deterministically.Since you’re already using fs + sass, replacing gulp copies with fs/promises (and awaiting) will simplify flow and remove race concerns.
src/checkbox/mixin.scss (1)
15-16: Prefer theme token over literal 'white' to support theming.Use a var.use-rgb(...) token (e.g., n-1) for foreground color instead of the hard-coded white.
- color: white; + color: var.use-rgb(n-1);src/dialog/confirm-dialog/confirm-dialog.component.scss (2)
4-7: Consider tokenizing local spacings.$padding: 32px and $action-padding: 20px could use spacing tokens for consistency with the rest of the file (e.g., var.use-var(spacing-xl?), var.use-var(spacing-l?)).
-$padding: 32px; -$action-padding: 20px; +$padding: var.use-var(spacing-xl); +$action-padding: var.use-var(spacing-l);
27-31: Confirm intent: primary and warning share the same icon color.Both &--primary and &--warning use var.use-rgb(yellow). If not intentional, consider mapping primary to primary token.
- &--primary, - &--warning { - color: var.use-rgb(yellow); - } + &--primary { + color: var.use-rgb(primary); + } + &--warning { + color: var.use-rgb(yellow); + }src/tabs/tab-header.component.scss (3)
79-82: Prefer theme tokens over hard-coded colors.box-shadow uses #ccc. Consider a neutral token to keep theming centralized.
- box-shadow: 0 0 4px #ccc; + box-shadow: 0 0 4px var.use-rgb(n-6);
121-124: Avoid removing focus outlines without a replacement on focusable containers.&__labels sets outline: none on :focus but no alternative style here. Verify that this element cannot receive focus or add a visible focus style for keyboard users.
- &__labels { + &__labels { display: flex; - - &:focus { - outline: none; - } + &:focus { + outline: none; + box-shadow: var.$tab-label-focus-box-shadow; // or another accessible indicator + } }
41-51: Optional: derive title metrics from tokens only.line-height: 32px is hard-coded while font-size, padding, etc. use tokens. Consider a var token for line-height for consistency.
- line-height: 32px; + line-height: var.use-var(line-height-l);src/switch/switch.component.scss (2)
11-11: Typo: $switch-dot-with should be $switch-dot-width.Minor readability/consistency fix; rename and update usages.
-$switch-dot-with: $switch-height - ($switch-border-width * 2); +$switch-dot-width: $switch-height - ($switch-border-width * 2);And update references:
- width: $switch-dot-with; - height: $switch-dot-with; + width: $switch-dot-width; + height: $switch-dot-width;- left: $switch-width - $switch-dot-with - $switch-border-width; + left: $switch-width - $switch-dot-width - $switch-border-width;
31-41: Consider honoring prefers-reduced-motion.Transitions on background-color/left could be wrapped in a media query to reduce motion when requested.
- transition: background-color var.$animation-duration-slow; + transition: background-color var.$animation-duration-slow; + @media (prefers-reduced-motion: reduce) { + transition: none; + }- transition: left var.$animation-duration-slow; + transition: left var.$animation-duration-slow; + @media (prefers-reduced-motion: reduce) { + transition: none; + }Also applies to: 43-53
src/radio/radio-button/radio-button.component.scss (1)
132-133: Optional: avoid magic font sizes on pointer icons.font-size: 10px/7px could be derived from icon size tokens for better scaling across themes.
- font-size: 10px; + font-size: calc(#{var.use-var(icon-size-m)} * 0.5);- font-size: 7px; + font-size: calc(#{var.use-var(icon-size-s)} * 0.5);Also applies to: 152-153
src/notification/notification.component.scss (3)
3-6: Import from package rather than node_modules path.Relative @use to ../../node_modules is brittle. Sass loaders resolve packages; import directly from the package.
-@use '../../node_modules/@angular/cdk/overlay-prebuilt' as *; +@use '@angular/cdk/overlay-prebuilt' as *;
14-15: Prefer tokenized radii.border-radius: 4px can use a theme radius for consistency.
- border-radius: 4px; + border-radius: var.use-var(border-radius-m);
7-9: Check z-index strategy.z-index: 1001 is a magic number. If the theme exposes overlay layers, align with that to avoid stacking issues.
- z-index: 1001; + z-index: var.$overlay-z-index; // if availableeslint.config.mjs (2)
116-123: ES language options likely too old for Angular 20 tooling.Set ecmaVersion to 'latest' and use module source type for TS contexts.
- languageOptions: { - ecmaVersion: 5, - sourceType: 'script', + languageOptions: { + ecmaVersion: 'latest', + sourceType: 'module',
110-112: Either enable Storybook rules or drop the import.You import eslint-plugin-storybook but don’t use it. Consider enabling its flat recommended config or removing the import to avoid dead code.
Option A (enable):
- // ...storybook.configs["flat/recommended"] + ...storybook.configs["flat/recommended"]Option B (remove import):
-import storybook from "eslint-plugin-storybook";Also applies to: 1-2
.storybook/reset-browser.scss (1)
1-2: Remove unused Sass module import.
mixinis not referenced in this file; drop it to avoid needless module load and future namespace drift.@use '../src/theme/var' as var; -@use '../src/theme/mixin' as mixin;src/dropdown/dropdown-button/dropdown-button.component.scss (1)
1-2: Drop unusedmixinimport.
mixinisn’t used here; removing it trims compile work and avoids accidental symbol leakage.@use '../../theme/var' as var; -@use '../../theme/mixin' as mixin;src/tag/check-tag/check-tag.component.scss (2)
2-3: Prefer namespaced @use overas *for consistency and to avoid identifier collisions.Most files in this PR use
as var/as mixin. Adopting the same here improves clarity and prevents global symbol spills.Minimal change to imports:
-@use '../../theme/var' as *; -@use '../../theme/mixin' as *; +@use '../../theme/var' as var; +@use '../../theme/mixin' as mixin;Then update usages in this file accordingly (examples):
use-rgb(n-1)→var.use-rgb(n-1)use-var(font-size-m)→var.use-var(font-size-m)calc(#{use-var(inline-height-s)} / 2)→calc(#{var.use-var(inline-height-s)} / 2)
1-1: Remove unusedsass:mathimport.No
math.APIs are used; safe to drop.-@use 'sass:math';.storybook/global.scss (1)
1-2: Avoidas *to reduce namespace collisions.No symbols are referenced here; prefer plain
@useto avoid polluting the scope.-@use './reset-browser' as *; -@use '../src/theme/style.scss' as *; +@use './reset-browser'; +@use '../src/theme/style.scss';src/tag/tag.component.scss (2)
2-3: Inconsistent module usage vs. rest of codebase.Other files use
@use ... as var/mixin;while this file usesas *. Consider aligning for consistency, or document why tags are exempt.-@use '../theme/var' as *; -@use '../theme/mixin' as *; +@use '../theme/var' as var; +@use '../theme/mixin' as mixin;Follow-up (if you adopt this): qualify calls, e.g.,
use-var(...) -> var.use-var(...),use-rgb(...) -> var.use-rgb(...),@include text-set(...) -> @include mixin.text-set(...).
1-1: Remove unusedsass:mathimport (no math. usage).*Keeps the stylesheet lean.
-@use 'sass:math'; +/* math not used */src/button/button-group/button-group.component.scss (1)
1-2: Drop unusedmixinimport.This file doesn’t include any mixins.
@use '../../theme/var' as var; -@use '../../theme/mixin' as mixin;src/icon/icon.component.scss (1)
1-1: Prefer namespaced Sass modules overas *for consistency and safetyUse explicit namespace (as used elsewhere in this PR) to avoid identifier collisions and to keep styles consistent.
Apply:
-@use '../theme/var' as *; +@use '../theme/var' as var;Follow-up: replace calls like
use-var(spacing-s)withvar.use-var(spacing-s)in this file.src/dropdown/menu-group/menu-group.component.scss (1)
1-2: Remove unused import
@use '../../theme/mixin' as mixin;isn’t referenced in this file.@use '../../theme/var' as var; -@use '../../theme/mixin' as mixin;src/autocomplete/suggestion/suggestion.component.scss (1)
1-3: Sass module migration looks correct; consider consistent quoting for selector arg.Elsewhere (e.g., option.component.scss) the mixin is called with a quoted string. For consistency, quote the selector here too.
-@include option.option(aui-suggestion); +@include option.option('aui-suggestion');src/form/form-item/form-item.component.scss (1)
92-101: Minor: avoid recomputing $map per iteration (micro).Not critical, but you can inline map.get directly when accessing width to reduce a temp binding.
- @each $key in large medium small { - $map: map.get($form-item-width, $key); - &--#{$key} { - .aui-form-item__control { - flex: 1; - max-width: map.get($map, width); - } - } - } + @each $key in large medium small { + &--#{$key} { + .aui-form-item__control { + flex: 1; + max-width: map.get(map.get($form-item-width, $key), width); + } + } + }src/date-picker/calendar/header/component.ts (1)
88-104: Prefer explicit destructuring over Object.values for start/end.This avoids reliance on property order and reads clearer.
- $headerRange = computed(() => { - const locale = this.i18nService.$locale(); - const [start, end] = Object.values( - calcRangeValue(this.$$dateNavRange(), this.$$anchor()), - ).map(date => date.toDate()); + $headerRange = computed(() => { + const locale = this.i18nService.$locale(); + const { start, end } = calcRangeValue( + this.$$dateNavRange(), + this.$$anchor(), + ); + const startDate = start.toDate(); + const endDate = end?.toDate(); return { start: { - year: start.toLocaleDateString(locale, { year: 'numeric' }), - month: start.toLocaleDateString(locale, { month: 'short' }), + year: startDate.toLocaleDateString(locale, { year: 'numeric' }), + month: startDate.toLocaleDateString(locale, { month: 'short' }), }, end: { - year: end?.toLocaleDateString(locale, { year: 'numeric' }), - month: end?.toLocaleDateString(locale, { month: 'short' }), + year: endDate?.toLocaleDateString(locale, { year: 'numeric' }), + month: endDate?.toLocaleDateString(locale, { month: 'short' }), }, }; });src/theme/motion/_zoom.scss (1)
11-18: Add prefers-reduced-motion fallback.Respect users’ reduced motion settings by minimizing animation when requested.
.#{$className}-enter { opacity: 0; animation-timing-function: var.$ease-out; } .#{$className}-leave { animation-timing-function: var.$ease-in-out; } + + @media (prefers-reduced-motion: reduce) { + .#{$className}-enter, + .#{$className}-leave { + animation-duration: 1ms; + animation-iteration-count: 1; + transition: none; + } + }src/checkbox/checkbox.component.scss (2)
1-3: Consider namespacing @use imports for consistency and collision-avoidance.Most files in this PR (e.g., radio, time-picker, card) use explicit namespaces (
as var,as mixin). Usingas *here is functional but less safe and inconsistent.Example refactor (illustrative — would require updating references in this file):
-@use '../theme/mixin' as *; -@use '../theme/var' as *; -@use './mixin' as *; +@use '../theme/mixin' as mixin; +@use '../theme/var' as var; +@use './mixin' as checkbox;And then:
use-var(...) -> var.use-var(...),use-rgb(...) -> var.use-rgb(...),use-text-color(...) -> var.use-text-color(...),@include text-set -> @include mixin.text-set,@include theme-dark -> @include mixin.theme-dark,@include pointer -> @include checkbox.pointer.
68-80: Avoid hard-coded 8px; make indeterminate mark size tokenized.Hard-coding
width/height: 8pxcan drift from theming scales. Prefer a variable or derive from an existing token.- width: 8px; - height: 8px; + $indeterminate-size: 8px; // TODO: replace with a theme token when available + width: $indeterminate-size; + height: $indeterminate-size;src/input/input.component.scss (1)
1-2: Optional: align with project-wide namespacing.This file still uses
as *for theme modules while others useas var/as mixin. Consider standardizing in a follow-up to reduce global symbol leakage.src/card/card.component.scss (1)
9-10: Consider replacing hard-coded padding with a spacing token.
padding: 20px;deviates from tokenized spacing used elsewhere. If a suitable token exists, prefer it for consistency.- padding: 20px; + padding: var.use-var(spacing-xxl); // or closest tokensrc/theme/style.scss (1)
4-4: Avoid deep node_modules path andas *on CSS-only import.Use the package import for the CDK prebuilt CSS and drop the namespace spread. This prevents brittle paths and aligns with Sass module rules.
Apply:
-@use '../../node_modules/@angular/cdk/overlay-prebuilt' as *; +@use '@angular/cdk/overlay-prebuilt';Alternatively, include the prebuilt CSS in angular.json "styles" to keep it global and out of Sass processing.
src/theme/index.scss (1)
4-9: Optionally narrow the public API surface.If some internals shouldn’t be public, consider
@forward 'var' show use-var, use-rgb;etc., to avoid exposing private symbols.src/status-bar/status-bar.component.scss (1)
86-96: Respect reduced motion preferences for the pending animation.Gate the animation under
prefers-reduced-motionand provide a non-animated fallback.Apply:
- &--pending { - height: var.use-var(inline-height-m); - animation: pending 0.5s linear infinite; + &--pending { + height: var.use-var(inline-height-m); + // Respect reduced motion + @media (prefers-reduced-motion: no-preference) { + animation: pending 0.5s linear infinite; + } + @media (prefers-reduced-motion: reduce) { + animation: none; + } background: repeating-linear-gradient( -60deg, var.use-rgb(b-5), var.use-rgb(b-5) var.use-var(spacing-s), var.use-rgb(b-6) var.use-var(spacing-s), var.use-rgb(b-6) var.use-var(spacing-m) ); }Also applies to: 104-112
src/autocomplete/autocomplete.component.scss (1)
1-3: Align module alias with other select files for consistency.Use
optionContainerto matchsrc/select/select.component.scss.Apply:
-@use '../select/option-container' as option-container; +@use '../select/option-container' as optionContainer; -@include option-container.option-container(aui-autocomplete); +@include optionContainer.option-container(aui-autocomplete);src/input/search/search.component.scss (1)
29-41: Reduce calc duplication for readability and maintainability.Define local helpers inside the loop to avoid repeating inline-padding-xs and icon-size arithmetic.
Apply:
@each $key in large medium small mini { $map: map.get(var.$input-size, $key); + $pad: var.use-var(inline-padding-xs); + $icon: map.get($map, icon-size); &--#{$key} { - .#{$block}__clear, - .#{$block}__spinner { - right: calc(#{$pad} + 1px); - } + .#{$block}__clear, + .#{$block}__spinner { right: calc(#{$pad} + 1px); } - .#{$block}__icon { left: calc(#{$pad} + 1px); } + .#{$block}__icon { left: calc(#{$pad} + 1px); } - .#{$block}__button-icon { padding: 0 var.use-var(inline-padding-xs); } + .#{$block}__button-icon { padding: 0 $pad; } &.hasIcon .#{$block}__input { - padding-left: calc(#{map.get($map, icon-size)} + #{$pad} * 2 + 1px); + padding-left: calc(#{$icon} + #{$pad} * 2 + 1px); } &.hasIcon.isSearching .#{$block}__input, &.hasIcon.isClearable .#{$block}__input { - padding-right: calc(#{map.get($map, icon-size)} + #{$pad} * 2 + 1px); + padding-right: calc(#{$icon} + #{$pad} * 2 + 1px); } &.hasButton .#{$block} { &__input { - padding-right: calc(#{map.get($map, icon-size)} + #{$pad} * 3 + 1px); + padding-right: calc(#{$icon} + #{$pad} * 3 + 1px); } &__clear { - right: calc(#{map.get($map, icon-size)} + #{$pad} * 3 + 1px * 2); + right: calc(#{$icon} + #{$pad} * 3 + 2px); } } &.hasButton.isClearable .#{$block}__input { - padding-right: calc(#{map.get($map, icon-size)} * 2 + #{$pad} * 4 + 3px); + padding-right: calc(#{$icon} * 2 + #{$pad} * 4 + 3px); }Also applies to: 43-60, 62-92
src/dropdown/menu/menu.component.scss (1)
2-2: Remove unused mixin import.Not referenced in this file.
-@use '../../theme/mixin' as mixin;src/input/input-group/input-group.component.scss (1)
52-55: Avoid hover styling when input is disabled.Guard the hover rule so disabled inputs don’t change border color.
- &:hover { - .aui-input { - border-color: var.use-rgb(primary); - } - } + &:hover { + .aui-input:not(:disabled) { border-color: var.use-rgb(primary); } + }Please confirm your input element uses the native disabled attribute; otherwise we can gate by a class instead.
scripts/build.js (1)
31-35: Copying overlay-prebuilt.css separately is a behavioral change.Consumers previously importing a single theme CSS may now miss overlay styles. Document this or concatenate into style.css to avoid a breaking change.
I can add either:
- A README note about importing theme/style.css and overlay-prebuilt.css, or
- A small concatenation step to append overlay CSS to theme/style.css during build.
src/date-picker/calendar/panel/picker-panel.style.scss (2)
19-19: Invalid value for justify-content
justify-content: stretch;is not a valid value and will be ignored. Remove it or replace with a valid property if stretching was intended.- justify-content: stretch; + /* removed: invalid value */
142-146: Border shorthand ordering (nit)Optional: use the conventional
width style colororder for readability.- border: var.use-rgb(primary) 1px solid; + border: 1px solid var.use-rgb(primary);src/table/table-scroll.scss (1)
139-143: Prefer :not(:last-child) for clarity.Selector works but is unconventional; simplify for readability.
- &:not(&:last-child) { + &:not(:last-child) {src/message/message.component.scss (1)
3-3: Avoid hardcoding node_modules in Sass @use path.Use package import to keep toolchain-agnostic.
-@use '../../node_modules/@angular/cdk/overlay-prebuilt' as *; +@use '@angular/cdk/overlay-prebuilt' as *;src/steps/steps.component.scss (1)
206-208: Avoid magic number (13px); use theme spacing.Replace fixed padding with a spacing token for consistency and theming.
&__info { - padding-bottom: 13px; + padding-bottom: var.use-var(spacing-s); }src/input/number-input/number-input.component.scss (1)
17-25: Replace hardcoded 132px with a theme variable.Hardcoding width harms themeability and responsive scaling.
&--small & { &__inner { - width: 132px; + /* Prefer a token; if unavailable, introduce form-item-width-xs */ + width: var.use-var(form-item-width-s); }If a dedicated form-item-width-xs is intended, add it to theme/var and use it here.
src/input/tags-input/mixin.scss (3)
1-1: Remove unused Sass module import.
sass:mathisn’t referenced; drop it to reduce churn.-@use 'sass:math'; +
62-104: DRY the size blocks via a parametric mixin.
&--large|medium|small|miniduplicate the same structure. Consider a helper mixin to centralize the calc/padding/text-set logic and pass the token (l|m|s|xs) to it. Keeps future token changes in one place.
22-31: Disabled state may still be interactive.If desired, add
pointer-events: none;on.isDisabledto prevent focus/clicks in edge cases where inner elements capture events.src/inline-alert/inline-alert.component.scss (1)
18-27: Avoid string slicing for palette lookup.Relying on
string.sliceto derive palette keys is brittle. Use an explicit map from color name to prefix; easier to read and safer if names change.@mixin color-of-types() { $types: ( primary: blue, success: green, warning: yellow, danger: red, error: red, info: blue, ); + // Map color name -> token prefix + $prefix-map: (blue: b, green: g, yellow: y, red: r); + @each $name, $value in $types { &--#{$name} { - border-color: var.use-rgb($value); - @include mixin.theme-light { - background-color: var.use-rgb(#{string.slice('#{$value}', 1, 1)}-7); - } - @include mixin.theme-dark { - background-color: var.use-rgb(#{string.slice('#{$value}', 1, 1)}-6); - } + $prefix: map-get($prefix-map, $value); + border-color: var.use-rgb($value); + @include mixin.theme-light { background-color: var.use-rgb(#{$prefix}-7); } + @include mixin.theme-dark { background-color: var.use-rgb(#{$prefix}-6); } }src/table/table.component.ts (2)
101-109: Sticky class override workaround acknowledged.Getter override is fine for now; add a TODO with link to upstream if there’s an issue tracking this in CDK for future cleanup.
87-99: UnusedenableScrollWrapperinput – safe to removeA repository‐wide search (TS, HTML, tests) found no references to
enableScrollWrapperbeyond its declaration insrc/table/table.component.ts(lines 87–89). It never drives a host binding, isn’t read in the template, nor passed in by any parent. You can safely delete this dead API.Locations to update:
- src/table/table.component.ts, remove lines 87–89
Suggested diff:
@Input() - enableScrollWrapper: boolean;src/theme/_var.scss (2)
14-19: Potential size-map inconsistency forsmall.
smallmaps tofont-size-m/line-height-mwhileminiuses-s. If unintentional, alignsmallto-s.Proposed change:
- font-size: base-var.use-var(font-size-m), - line-height: base-var.use-var(line-height-m), + font-size: base-var.use-var(font-size-s), + line-height: base-var.use-var(line-height-s),Also applies to: 21-25, 27-31, 33-37
70-81: Typo:$date-picker-seprator-gap.
Keep for BC but add a correctly spelled alias to reduce future mistakes.Suggested addition (non-breaking):
+$date-picker-separator-gap: $date-picker-seprator-gap;src/paginator/paginator.component.scss (1)
75-85: Add focus-visible styling for keyboard a11y (optional).
Navigator buttons have hover but no focus ring; addoutline-shadowfor:focus-visible.&__navigator.aui-button { @@ &:hover:not([disabled]) { background-color: var.use-rgb(p-6); } + &:focus-visible { + @include mixin.outline-shadow(primary); + } }src/time-picker/panel/panel.style.scss (1)
62-86: States (hover/selected/disabled) are well-tokenized; consider keyboard focus (optional).
Add:focus-visibleto.#{block}__cellfor accessibility.&__cell { @@ &:hover { background-color: var.use-rgb(p-6); color: var.use-rgb(primary); } + &:focus-visible { + outline: none; + @include mixin.outline-shadow(primary); + }src/drawer/component/internal/internal.component.scss (1)
7-18: Consider using position: fixed for the mask to ensure full-viewport coverage.Absolute + height:100% depends on a positioned ancestor; can fail if container sizing changes or on page scroll. Fixed is more robust for overlays.
Apply if acceptable:
- .#{$drawer}-mask { - position: absolute; + .#{$drawer}-mask { + position: fixed; top: 0; left: 0; width: 100%; - height: 0; + height: 0; @include mixin.modal-backdrop;src/select/multi-select/multi-select.component.scss (1)
17-22: Remove or use the computed $padding to avoid duplication.$padding is computed but never used; padding-right repeats the same calc.
Apply one of:
- $padding: calc( - #{var.use-var(inline-padding-xs)} * 2 + #{map.get($map, icon-size)} + 1px - ); + $padding: calc( + #{var.use-var(inline-padding-xs)} * 2 + #{map.get($map, icon-size)} + 1px + );and later:
- padding-right: calc( - #{var.use-var(inline-padding-xs)} * - 2 + - #{map.get($map, icon-size)} + - 1px - ); + padding-right: $padding;Or, if you prefer not to keep the variable, drop its declaration.
Also applies to: 23-38
src/button/button.component.scss (2)
20-26: Verify “small” sizing tokens; looks inconsistent with other sizes.Small uses font-size-m, line-height-m, and icon-size-m whereas mini uses -s. If not intentional, switch small to -s tokens.
Proposed change (only if it matches design):
small: ( - height: var.use-var(inline-height-s), - padding-hoz: var.use-var(inline-padding-s), - font-size: var.use-var(font-size-m), - line-height: var.use-var(line-height-m), - icon-size: var.use-var(icon-size-m), + height: var.use-var(inline-height-s), + padding-hoz: var.use-var(inline-padding-s), + font-size: var.use-var(font-size-s), + line-height: var.use-var(line-height-s), + icon-size: var.use-var(icon-size-s), ),And in text-button-sizes:
&.aui-button--small:not(.isSquare) { - height: var.use-var(line-height-m); - line-height: var.use-var(line-height-m); - font-size: var.use-var(font-size-m); + height: var.use-var(line-height-s); + line-height: var.use-var(line-height-s); + font-size: var.use-var(font-size-s); }Also applies to: 36-60, 163-186
62-123: Optional: theme-friendly text color for colorful buttons.Hardcoded color: white may not respect themes. If available, consider var.use-text-color(on-primary)/inverse.
- color: white; + // If token exists: + color: var.use-text-color(on-primary);src/dialog/dialog.component.scss (1)
97-118: Prefer semantic state tokens over raw palette indices for hover/active.Using
p-6/p-5/p-0couples the component to the raw palette and can regress across themes (dark mode, high-contrast). If your design system exposes semantic tokens (e.g., button/icon hover/active backgrounds), switch to those.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
.yarn/install-state.gzis excluded by!**/.yarn/**,!**/*.gzyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (107)
.storybook/global.scss(1 hunks).storybook/main.ts(0 hunks).storybook/manager.ts(1 hunks).storybook/preview.ts(1 hunks).storybook/reset-browser.scss(4 hunks).stylelintrc(1 hunks).yarnrc.yml(1 hunks)eslint.config.mjs(2 hunks)package.json(5 hunks)scripts/build.js(4 hunks)scripts/copy-resources.js(2 hunks)src/accordion/accordion-item/accordion-item.component.scss(2 hunks)src/accordion/accordion.component.scss(1 hunks)src/anchor/anchor.component.scss(6 hunks)src/anchor/anchor.directive.ts(0 hunks)src/autocomplete/autocomplete.component.scss(1 hunks)src/autocomplete/suggestion-group/suggestion-group.component.scss(1 hunks)src/autocomplete/suggestion/suggestion.component.scss(1 hunks)src/back-top/back-top.component.scss(1 hunks)src/breadcrumb/breadcrumb-item.component.scss(1 hunks)src/breadcrumb/breadcrumb.component.scss(1 hunks)src/button/button-group/button-group.component.scss(3 hunks)src/button/button.component.scss(6 hunks)src/card/card.component.scss(2 hunks)src/card/section.component.scss(1 hunks)src/checkbox/checkbox.component.scss(1 hunks)src/checkbox/mixin.scss(1 hunks)src/color-picker/color-picker.component.scss(2 hunks)src/date-picker/calendar/date-picker-panel/style.scss(1 hunks)src/date-picker/calendar/footer/style.scss(1 hunks)src/date-picker/calendar/header/component.ts(5 hunks)src/date-picker/calendar/header/style.scss(2 hunks)src/date-picker/calendar/panel/picker-panel.style.scss(6 hunks)src/date-picker/calendar/range-picker-panel/style.scss(3 hunks)src/date-picker/range-picker/range-picker.style.scss(1 hunks)src/date-picker/trigger/trigger.style.scss(5 hunks)src/dialog/confirm-dialog/confirm-dialog.component.scss(2 hunks)src/dialog/dialog-content/dialog-content.component.ts(1 hunks)src/dialog/dialog.component.scss(3 hunks)src/drawer/component/internal/internal.component.scss(6 hunks)src/dropdown/dropdown-button/dropdown-button.component.scss(2 hunks)src/dropdown/menu-group/menu-group.component.scss(1 hunks)src/dropdown/menu-item/menu-item.component.scss(1 hunks)src/dropdown/menu-item/mixin.scss(3 hunks)src/dropdown/menu/menu.component.scss(1 hunks)src/dropdown/submenu/submenu.component.scss(3 hunks)src/form/form-item/form-item.component.scss(6 hunks)src/icon/icon.component.scss(1 hunks)src/inline-alert/inline-alert.component.scss(4 hunks)src/input/input-group/input-group.component.scss(4 hunks)src/input/input.component.scss(2 hunks)src/input/number-input/number-input.component.scss(7 hunks)src/input/search/search.component.scss(5 hunks)src/input/tags-input/mixin.scss(6 hunks)src/input/tags-input/tags-input.component.scss(1 hunks)src/message/base-message.ts(0 hunks)src/message/message-wrapper.component.scss(0 hunks)src/message/message.component.scss(2 hunks)src/message/message.service.ts(0 hunks)src/notification/notification-wrapper.component.scss(0 hunks)src/notification/notification.component.scss(3 hunks)src/notification/notification.config.ts(1 hunks)src/notification/notification.service.ts(0 hunks)src/paginator/paginator.component.scss(4 hunks)src/radio/radio-button/radio-button.component.scss(6 hunks)src/radio/radio.component.scss(5 hunks)src/select/helper-directives.ts(0 hunks)src/select/multi-select/multi-select.component.scss(2 hunks)src/select/option-container.scss(1 hunks)src/select/option-group/option-group-style.scss(1 hunks)src/select/option-group/option-group.component.scss(1 hunks)src/select/option/option-style.scss(1 hunks)src/select/option/option.component.scss(1 hunks)src/select/select.component.scss(2 hunks)src/select/validators.ts(1 hunks)src/sort/sort-header.component.scss(3 hunks)src/status-bar/status-bar.component.scss(3 hunks)src/steps/steps.component.scss(9 hunks)src/switch/switch.component.scss(5 hunks)src/table/table-scroll.scss(10 hunks)src/table/table.component.scss(8 hunks)src/table/table.component.ts(3 hunks)src/tabs/tab-group.component.scss(0 hunks)src/tabs/tab-header.component.scss(9 hunks)src/tag/check-tag/check-tag.component.scss(1 hunks)src/tag/tag.component.scss(1 hunks)src/theme/_mixin.scss(12 hunks)src/theme/_pattern.scss(2 hunks)src/theme/_theme-preset.scss(1 hunks)src/theme/_var.scss(2 hunks)src/theme/index.scss(1 hunks)src/theme/motion/_fade.scss(1 hunks)src/theme/motion/_zoom.scss(1 hunks)src/theme/motion/index.scss(1 hunks)src/theme/style.scss(3 hunks)src/time-picker/panel/panel.style.scss(5 hunks)src/time-picker/style.scss(1 hunks)src/tooltip/tooltip.component.scss(1 hunks)src/tree-select/tree-node.component.scss(3 hunks)src/tree-select/tree-select.component.scss(1 hunks)stories/anchor/anchor.mdx(1 hunks)stories/autocomplete/autocomplete.mdx(1 hunks)stories/back-top/back-top.mdx(1 hunks)stories/button/button.mdx(1 hunks)stories/card/card.mdx(1 hunks)stories/checkbox/checkbox.mdx(1 hunks)stories/date-picker/date-picker.mdx(1 hunks)
⛔ Files not processed due to max files limit (28)
- stories/dialog/dialog.mdx
- stories/drawer/drawer.mdx
- stories/dropdown/dropdown.mdx
- stories/form/form.mdx
- stories/icon/icon.mdx
- stories/inline-alert/inline.mdx
- stories/input/input.mdx
- stories/paginator/paginator.mdx
- stories/pattern/global-style.component.scss
- stories/pattern/pattern.mdx
- stories/pattern/shadow.component.scss
- stories/radio/radio.mdx
- stories/rangepicker/rangepicker.mdx
- stories/scrolling/scrolling.mdx
- stories/select/select.mdx
- stories/steps/steps.mdx
- stories/switch/switch.mdx
- stories/table/placeholder.component.scss
- stories/table/sticky-columns.component.scss
- stories/table/table.mdx
- stories/tabs/tabs.mdx
- stories/tag/tag.mdx
- stories/theme/palette.component.scss
- stories/theme/theme.mdx
- stories/timepicker/timepicker.mdx
- stories/tooltip/tooltip.mdx
- tsconfig.json
- tsconfig.lib.json
💤 Files with no reviewable changes (9)
- src/notification/notification-wrapper.component.scss
- src/message/message-wrapper.component.scss
- .storybook/main.ts
- src/tabs/tab-group.component.scss
- src/message/message.service.ts
- src/message/base-message.ts
- src/anchor/anchor.directive.ts
- src/notification/notification.service.ts
- src/select/helper-directives.ts
🧰 Additional context used
🧬 Code graph analysis (1)
scripts/build.js (1)
scripts/utils.js (2)
fs(1-1)path(2-2)
🪛 GitHub Actions: Run tests
package.json
[error] 1-1: Yarn version mismatch: packageManager field in package.json specifies yarn@4.0.1, but the global Yarn version is 1.22.22. Corepack must be enabled by running 'corepack enable'.
🪛 GitHub Actions: Release beta
package.json
[error] 1-1: Step 'yarn cache dir' failed: Yarn version mismatch. package.json defines "packageManager": "yarn@4.0.1" but the global Yarn version is 1.22.22. Corepack must be enabled (run 'corepack enable').
🪛 Biome (2.1.2)
eslint.config.mjs
[error] 148-148: This property is later overwritten by an object member with the same name.
Overwritten with this property.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property.
(lint/suspicious/noDuplicateObjectKeys)
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/release-beta.yml (1)
12-35: Action Required: Add Corepack Enable to All Yarn WorkflowsOur audit shows that only release-beta.yml includes the
corepack enablestep. All other workflows that invoke Yarn are missing this, which causes the Yarn v1/v4 mismatch in your “Run tests” job. Please insert the Corepack enable step before anyactions/setup-node(and thus before any Yarn caching or invocation) in each of the following workflows:
- .github/workflows/test.yml
- .github/workflows/stories.yml
- .github/workflows/release.yml
- .github/workflows/release-prod.yml
For each file, add these lines immediately after checkout and before setup-node:
- name: Checkout Repo uses: actions/checkout@v3 + - name: Corepack Enable + run: corepack enable - name: Setup Node.js uses: actions/setup-node@v3 with: node-version: 20 cache: yarnThis guarantees Corepack is active prior to any Yarn use, preventing version mismatches.
♻️ Duplicate comments (1)
package.json (1)
12-12: Yarn 4 pin looks good; consider a lightweight local guard.Optionally add a preinstall to auto-enable Corepack locally to match CI.
{ "scripts": { "preinstall": "corepack enable || true" } }
🧹 Nitpick comments (2)
.github/workflows/release-beta.yml (1)
24-26: Use Yarn 4 syntax for immutable installs.Prefer Yarn 4’s flag; the old alias may be removed later.
- - name: Install dependencies - run: yarn --frozen-lockfile + - name: Install dependencies + run: yarn install --immutableFor reference: Yarn docs note --frozen-lockfile is just a backward-compat alias and may be removed. (yarnpkg.com)
package.json (1)
106-107: Check necessity of webpack-only loaders with Vite-based builds.If Webpack is only used by Storybook, keeping sass-loader and style-loader is fine; otherwise consider removing unused loaders to reduce dev surface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
.yarn/install-state.gzis excluded by!**/.yarn/**,!**/*.gzyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
.github/workflows/release-beta.yml(1 hunks)package.json(5 hunks)
🧰 Additional context used
🪛 GitHub Actions: Run tests
package.json
[error] 1-1: Yarn version mismatch: packageManager field in package.json specifies "yarn@4.0.1", but the global Yarn version is 1.22.22. Corepack must be enabled to use Yarn 4; run 'corepack enable'. The failing step: 'yarn cache dir'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Redirect rules - aui
- GitHub Check: Header rules - aui
- GitHub Check: Pages changed - aui
🔇 Additional comments (7)
.github/workflows/release-beta.yml (2)
15-17: Good call adding Corepack.This unblocks Yarn 4 in CI.
18-23: Upgrade setup-node and pin Node >=20.19 to match Angular 20.2 requirements.
- Use actions/setup-node@v4 (better Yarn Berry support/caching).
- Ensure Node is at least 20.19.0 (Angular 20 requires it).
- - name: Setup Node.js - uses: actions/setup-node@v3 + - name: Setup Node.js + uses: actions/setup-node@v4 with: - node-version: 20 - cache: yarn + node-version: 20.19.x + cache: yarn + cache-dependency-path: yarn.lockAngular 20.0.x requires TS <5.9; Angular 20.2 adds TS 5.9 support, but both require Node ^20.19.0. Please confirm your runners pick >=20.19.0. Sources: Angular version table; 20.2 notes. (angular.dev, blog.ninja-squad.com)
package.json (5)
53-64: Angular 20.2 dependency bumps LGTM.Versions are consistent across framework packages.
77-80: Storybook 9 upgrade noted.Ensure your Storybook builder/config is migrated (which this PR appears to cover). No action here.
101-101: ng-packagr v20 aligns with Angular 20.Looks consistent.
114-114: tslib bump is fine.Matches Angular/TS toolchain expectations.
115-116: TypeScript 5.9.2 is compatible with Angular 20.2 — good to go.Angular v20.2 adds TS 5.9 support, resolving earlier concerns about 5.9 on 20.0.x.
For reference: v20.2 TS 5.9 support. (blog.ninja-squad.com)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
.github/workflows/test.yml (1)
13-15: Move Corepack after setup-node; switch to Yarn 4 immutable install; consider actions v4 + better cache keys.
- Run Corepack after “Setup Node.js” so the just-installed Node’s Corepack shims are used.
- With Yarn 4, prefer
yarn install --immutableoveryarn --frozen-lockfile.- Consider upgrading to
actions/checkout@v4andactions/setup-node@v4, and addcache-dependency-pathfor more stable caching.Here’s a minimal reorder (outside the selected lines) and command updates:
# … previous steps … - name: Setup Node.js uses: actions/setup-node@v4 with: node-version: 20 cache: yarn cache-dependency-path: | **/yarn.lock - name: Corepack Enable run: corepack enable - name: Install Dependencies run: yarn install --immutableIf you prefer diffs for the two command updates:
- uses: actions/setup-node@v3 + uses: actions/setup-node@v4 @@ - run: yarn --frozen-lockfile + run: yarn install --immutable.github/workflows/release.yml (1)
19-21: Same here: place Corepack after setup-node; adopt Yarn 4’s immutable install; tighten action versions and cache.
- Order: setup-node → corepack enable → install.
- Yarn: use
yarn install --immutable.- Actions: consider
checkout@v4,setup-node@v4, addcache-dependency-path.- Security: job-level least-privilege permissions for Changesets.
Suggested edits (outside the selected lines):
permissions: contents: write pull-requests: write steps: - name: Checkout Repo uses: actions/checkout@v4 with: fetch-depth: 0 - name: Setup Node.js uses: actions/setup-node@v4 with: node-version: 20 cache: yarn cache-dependency-path: | **/yarn.lock - name: Corepack Enable run: corepack enable - name: Install Dependencies run: yarn install --immutable.github/workflows/stories.yml (1)
15-17: Reorder Corepack after setup-node; use Yarn 4 immutable install; consider modern Pages deploy + permissions.
- Switch to setup-node → corepack enable → yarn install.
- Use
yarn install --immutable.- Optional: migrate from
crazy-max/ghaction-github-pages@v1to the official Pages actions orpeaceiris/actions-gh-pages@v3.- Add minimal permissions for Pages.
Proposed snippet (outside the selected lines):
permissions: contents: read pages: write id-token: write steps: - name: Checkout Repo uses: actions/checkout@v4 - name: Setup Node.js uses: actions/setup-node@v4 with: node-version: 20 cache: yarn cache-dependency-path: | **/yarn.lock - name: Corepack Enable run: corepack enable - name: Install dependencies run: yarn install --immutable.github/workflows/release-prod.yml (1)
18-20: Align step order with Yarn 4 best practices; upgrade actions; immutable install; add least-privilege perms.
- Place Corepack after setup-node.
- Use
yarn install --immutable.- Consider
checkout@v4,setup-node@v4, and cache dependency path.- Add
permissions: contents: writefor the release job.Suggested changes (outside the selected lines):
permissions: contents: write steps: - name: Checkout Repo uses: actions/checkout@v4 - name: Setup Node.js uses: actions/setup-node@v4 with: node-version: 20 cache: yarn cache-dependency-path: | **/yarn.lock - name: Corepack Enable run: corepack enable - name: Install dependencies run: yarn install --immutable
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
.github/workflows/release-prod.yml(1 hunks).github/workflows/release.yml(1 hunks).github/workflows/stories.yml(1 hunks).github/workflows/test.yml(1 hunks)scripts/publish-version.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Release Beta
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
package.json (2)
12-12: CI: Ensure Corepack is enabled for Yarn 4.Yarn 4 expects Corepack; add
corepack enable(and optionallycorepack prepare yarn@4.0.1 --activate) before any yarn command in CI. Docs: Corepack must be enabled on runners. (yarnpkg.com)
43-46: Tighten RxJS peer range to match Angular 20 minimum.Angular 20 supports RxJS ^6.5.3 or ≥7.4; requiring
^7.4.0prevents unsupported 7.0–7.3. (angular.dev)"peerDependencies": { "@angular/cdk": ">=20.0.0", "dayjs": "^1.0.0", - "rxjs": "^7.0.0" + "rxjs": "^7.4.0" },
🧹 Nitpick comments (4)
scripts/run-lint.js (1)
11-14: Rename localprocessto avoid confusion with Node’s global.Minimal change if you keep the current structure:
- const process = spawn(cmd, args, { + const child = spawn(cmd, args, { stdio: 'inherit', shell: true }); - process.on('close', (code) => { + child.on('close', (code) => {package.json (3)
26-29: Dropnpxfor Yarn 4 scripts; use local bins directly.Yarn v4 exposes project bins on PATH during scripts;
npxcan slow or bypass PnP. Align with run-lint:- "lint": "node scripts/run-lint.js", - "lint:es": "npx eslint . --cache -f friendly", - "lint:style": "npx stylelint --cache '{src,stories}/**/*.{html,scss}'", - "lint:tsc": "npx tsc -p . --noEmit", + "lint": "node scripts/run-lint.js", + "lint:es": "eslint . --cache -f friendly", + "lint:style": "stylelint --cache '{src,stories}/**/*.{html,scss}'", + "lint:tsc": "tsc -p . --noEmit",
114-116: Consider upgradingtypescript-eslintto v8 for ESLint 9.ESLint 9 is in devDeps;
typescript-eslintv8 ships full ESLint 9 support. Consider^8.xto avoid edge incompatibilities. (typescript-eslint.io)- "typescript-eslint": "7.16.1", + "typescript-eslint": "^8.0.0",
1-41: Optional: declare Node engines to match Angular 20 requirements.Add this to advertise the required runtime (helps CI and consumers). (angular.dev)
{ "engines": { "node": "^20.19.0 || ^22.12.0 || ^24.0.0" } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
.yarn/install-state.gzis excluded by!**/.yarn/**,!**/*.gzyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
package.json(6 hunks)scripts/run-lint.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Redirect rules - aui
- GitHub Check: Header rules - aui
- GitHub Check: Pages changed - aui
- GitHub Check: Unit Test
- GitHub Check: Release Beta
🔇 Additional comments (1)
package.json (1)
53-65: TS 5.9 with Angular 20.2: OK; keep Node ≥20.19.Angular v20.2 adds TypeScript 5.9 support; your pins (^20.2.x + ~5.9.2) are consistent. Ensure CI uses a supported Node (≥20.19 or 22.12/24.0). (blog.ninja-squad.com, angular.dev)
Also applies to: 115-115
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/test.yml (2)
13-15: Corepack step placement is correct; consider pinning Yarn via Corepack.Nice addition and ordering (right after checkout). To guarantee the exact Yarn version CI uses, either ensure package.json has a packageManager field like "yarn@4.x.x" or explicitly activate it.
Optional tweak:
- name: Corepack Enable run: corepack enable + - name: Use repo's Yarn version + if: ${{ hashFiles('package.json') != '' }} + run: | + node -e "const p=require('./package.json'); if(!p.packageManager) process.exit(1)" + corepack use
28-29: Update CI to use Yarn v4’s--immutableflag and unify package manager usageYarn v4 still supports
--frozen-lockfileonly as a deprecated alias (it will be removed in a future release) and now recommends using--immutableinstead to avoid warnings and ensure reproducible installs (yarnpkg.com). Since you’ve explicitly setnodeLinker: node-modulesin.yarnrc.yml, PnP is disabled and mixing npm/Yarn for scripts won’t break today—but it’s clearer and more future-proof to stick with one tool across CI.Please update the following in
.github/workflows/test.yml:
- Replace the deprecated install flag
- (Optionally) run tests through Yarn rather than npm
- - name: Install Dependencies - run: yarn --frozen-lockfile + - name: Install Dependencies + run: yarn install --immutable @@ - - name: Run test - run: npm run test:ci + - name: Run test + run: yarn test:ci
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/test.yml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Redirect rules - aui
- GitHub Check: Header rules - aui
- GitHub Check: Pages changed - aui
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/dialog/dialog.service.spec.ts (1)
77-88: Make the “cancel button closes dialog” test async-safe.Currently it can pass without executing expectations. Either return a Promise or use fakeAsync/tick.
-it('should be closed by click cancel button', () => { +it('should be closed by click cancel button', fakeAsync(() => { const dialogRef = dialogService.open(TestDialogContentComponent, { noAnimation: true, }); - dialogRef.afterClosed().subscribe(result => { - expect(result).toBeFalsy(); - expect(ocEl.querySelector('aui-dialog')).toBeNull(); - }); + let closed: any; + dialogRef.afterClosed().subscribe(r => (closed = r)); fixture.detectChanges(); - ocEl.querySelector('#cancel')?.dispatchEvent(new Event('click')); -}); + const cancelBtn = ocEl.querySelector('#cancel') as HTMLButtonElement | null; + expect(cancelBtn).not.toBeNull(); + cancelBtn!.dispatchEvent(new Event('click')); + tick(); + expect(closed).toBe(false); + expect(ocEl.querySelector('aui-dialog')).toBeNull(); +}));
🧹 Nitpick comments (7)
.yarnrc.yml (1)
1-1: nodeLinker is OK; add immutable installs for CI determinismUsing node-modules is compatible with Angular/Storybook. Recommend locking installs in CI to avoid drift.
Apply:
nodeLinker: node-modules +enableImmutableInstalls: truejest.setup.ts (2)
27-31: Prefer globalThis for cross-runner stabilityUse
globalThisinstead ofglobalto avoid Node-only globals and align with standard JS environments.-global.ResizeObserver = jest.fn().mockImplementation(() => ({ +globalThis.ResizeObserver = jest.fn().mockImplementation(() => ({ observe: jest.fn(), unobserve: jest.fn(), disconnect: jest.fn(), }));
12-25: Make matchMedia mock configurable to ease redefinition in testsAdding
configurable: truehelps when individual tests need to redefine or restorematchMedia.Object.defineProperty(window, 'matchMedia', { - writable: true, + writable: true, + configurable: true, value: jest.fn().mockImplementation(query => ({ matches: false, media: query,src/dialog/dialog.service.spec.ts (4)
55-58: Assert element existence before checking classList.Improves failure messages and prevents undefined received values.
-expect(ocEl.querySelector('.aui-dialog')?.classList).toContain( - 'aui-dialog--large', -); +const dialogEl1 = ocEl.querySelector('.aui-dialog'); +expect(dialogEl1).not.toBeNull(); +expect(dialogEl1!.classList).toContain('aui-dialog--large');-expect(ocEl.querySelector('.aui-dialog')?.classList).toContain( - 'test-class', -); +const dialogEl2 = ocEl.querySelector('.aui-dialog'); +expect(dialogEl2).not.toBeNull(); +expect(dialogEl2!.classList).toContain('test-class');Also applies to: 71-74
95-98: Avoid innerHTML in assertions; prefer textContent and explicit existence check.Less brittle and safer.
-expect(ocEl.querySelector('.aui-dialog button')?.innerHTML).toContain( - 'close', -); +const closeBtn = ocEl.querySelector('.aui-dialog button'); +expect(closeBtn).not.toBeNull(); +expect(closeBtn!.textContent).toContain('close');
152-156: Same here: use textContent and check elements exist.-expect( - ocEl.querySelector('.aui-confirm-dialog__title')?.innerHTML, -).toContain('custom title'); -expect( - ocEl.querySelector('.aui-confirm-dialog__content')?.innerHTML, -).toContain('custom content'); +const titleEl = ocEl.querySelector('.aui-confirm-dialog__title'); +const contentEl = ocEl.querySelector('.aui-confirm-dialog__content'); +expect(titleEl).not.toBeNull(); +expect(contentEl).not.toBeNull(); +expect(titleEl!.textContent).toContain('custom title'); +expect(contentEl!.textContent).toContain('custom content');
182-184: Remove stray console.log in tests.Keep test output clean.
- console.log('confirm', confirmBtn);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (9)
.yarn/install-state.gzis excluded by!**/.yarn/**,!**/*.gz.yarn/plugins/plugin-prepare-lifecycle.cjsis excluded by!**/.yarn/**src/breadcrumb/__snapshots__/breadcrumb.component.spec.ts.snapis excluded by!**/*.snapsrc/form/__snapshots__/form.component.spec.ts.snapis excluded by!**/*.snapsrc/input/input-group/__snapshots__/input-group.component.spec.ts.snapis excluded by!**/*.snapsrc/paginator/__snapshots__/paginator.component.spec.ts.snapis excluded by!**/*.snapsrc/status-bar/__snapshots__/status-bar.component.spec.ts.snapis excluded by!**/*.snapsrc/tag/__snapshots__/tag.component.spec.ts.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
.yarnrc.yml(1 hunks)jest.setup.ts(1 hunks)package.json(6 hunks)patches/cliui+8.0.1.patch(0 hunks)src/dialog/dialog.service.spec.ts(12 hunks)
💤 Files with no reviewable changes (1)
- patches/cliui+8.0.1.patch
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Unit Test
- GitHub Check: Release Beta
🔇 Additional comments (3)
.yarnrc.yml (1)
3-3: Use official npm registry as default and rely on Yarn’s env var override
Update.yarnrc.yml:-npmRegistryServer: "https://registry.npmmirror.com" +npmRegistryServer: "https://registry.npmjs.org"Yarn supports overriding simple settings via
YARN_<KEY>environment variables (e.g.YARN_NPM_REGISTRY_SERVER) (yarnpkg.com). Verify that all CI pipelines setYARN_NPM_REGISTRY_SERVER=https://registry.npmmirror.comin region-specific runners (currently no override in repo).src/dialog/dialog.service.spec.ts (2)
182-182: Non-null assertions on critical buttons are appropriate.These make failures loud when selectors break.
Also applies to: 211-211, 242-242, 296-296, 300-300
199-201: Ensure confirm consumes only the first emission
timer(100, 100)produces an infinite, non-completing Observable. Unless theconfirmimplementation explicitly takes a single value (e.g. viafirstValueFromor.pipe(take(1))), the promise will never resolve. Replace with a one-shottimer(100)or wrap the periodic timer withtake(1).
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
package.json (1)
84-84: Upgradeangular-eslintto version 21.x to match Angular 21.The project uses Angular 21.1.1, but
angular-eslintis pinned to version 19.1.0 (2 major versions behind). The latestangular-eslintversion (21.1.0) explicitly requires@angular/cli >= 21.0.0, and version alignment betweenangular-eslintand Angular is a best practice for compatibility and consistent linting behavior. Upgrade to at leastangular-eslint@^21.1.0.src/date-picker/calendar/panel/picker-panel.template.html (1)
22-56: Improve tracking efficiency in@forloops.
track trackByFn()returnsthis.navRange(a constant), causing all rows and items to share the same track key. This defeats change detection optimization—items won't be properly tracked as they move. Usetrack rowfor the outer loop andtrack item(ortrack item.value) for the inner loop, or modifytrackByFnto accept and use index/item parameters.
🤖 Fix all issues with AI agents
In `@src/accordion/accordion-item/accordion-item.component.html`:
- Around line 19-33: The template currently uses `@if` (expanded) which removes
the accordion body from the DOM and prevents the state-based expand animation
([`@expand`] using states like 'expanded' and '*') from running on collapse;
replace the conditional removal with persistent markup and toggle state instead:
remove the `@if` (expanded) wrapper so the <div class="aui-accordion-item__body"
[`@expand`]="expanded ? 'expanded' : 'collapsed'" [id]="id"> is always rendered,
and control visibility/accessibility via a binding (e.g., [hidden]="!expanded"
or [attr.aria-hidden]="!expanded") while keeping the inner content/ng-template
(lazyContentTpl) as-is so the expand animation on the "expanded" -> "*"
transition can execute correctly.
In `@src/date-picker/trigger/trigger.template.html`:
- Around line 41-52: The input in the single-date template declares
auiTooltipType twice (auiTooltipType="plain" and auiTooltipType="info"), which
must be deduplicated; remove the first auiTooltipType="plain" from the input
element so only auiTooltipType="info" remains on that <input> (the element with
`#focusRef`, [value]="$any($formatValue())", [readonly]="true", and [placeholder]
bindings).
In `@src/input/tags-input/tags-input.component.html`:
- Around line 13-25: The track expression uses `index` before it's defined,
causing a runtime error; update the template to use the implicit `$index` in the
track call (e.g. change `track trackByValue(index, tag)` to `track
trackByValue($index, tag)`) and keep `let index = $index` for the
`onRemove(index)` handler (or switch `onRemove` to call with `$index` directly)
so `trackByValue` and `onRemove` refer to a defined index; target the
`trackByValue` usage in the `@for` loop and the `onRemove` call in the
`tags-input.component.html`.
In `@src/select/multi-select/multi-select.component.html`:
- Around line 41-60: The `@for` loop in multi-select.component.html uses an
invalid function call in its track expression (track trackByValue($index,
option)); change the track expression to a property reference such as track
option.value so Angular accepts it—update the `@for` iterating selectedOptions$ |
async to use "track option.value" instead of invoking trackByValue; leave the
rest of the tag rendering (aui-tag, isTemplateRef handling, removeValue)
unchanged.
In `@stories/form/basic.component.ts`:
- Around line 138-172: The template's tags are mis-indented and mis-nested
starting after the </aui-radio> close: reformat the block so closing tags for
</aui-radio-group>, </aui-form-item> and the next <aui-form-item> sit at the
same 2-space indentation level as their opening tags; ensure the
aui-checkbox-group and its child <aui-checkbox> elements are nested directly
under their <aui-form-item> parent (check aui-radio-group, aui-checkbox-group,
</aui-radio-group>, </aui-form-item> and </aui-checkbox-group> placements) and
fix any stray extra spaces/newlines so the entire template uses consistent
2-space indentation and correct parent/child nesting.
In `@stories/toc/basic.component.ts`:
- Around line 66-73: TocBasicComponent's template calls remove(item) but the
class lacks that method; add a click handler named remove with the appropriate
signature (e.g., remove(item: string | YourItemType): void) to TocBasicComponent
so template type-checks and runtime clicks won't fail, and implement the body to
perform the intended action (no-op, remove from list, or emit an event)
consistent with the component's behavior; alternatively remove the
(click)="remove(item)" binding from the template if the click should do nothing.
♻️ Duplicate comments (4)
package.json (4)
12-12: CI requires Corepack setup for Yarn 4.This issue was previously flagged. Ensure your CI workflow enables Corepack before running yarn commands.
30-30: Use built-inyarn dedupeinstead ofyarn-deduplicate.This was previously flagged. The
preparescript no longer callsyarn-deduplicate, but the package is still in devDependencies (line 118). Consider either:
- Removing
yarn-deduplicatefrom devDependencies if unused- Or adding
yarn dedupe --strategy fewerback to the prepare script for Yarn 4 compatibility
43-45: Tighten RxJS peer range to^7.4.0.This was previously flagged. Angular 20+ requires RxJS >=7.4. The current
^7.0.0allows unsupported versions.
115-116: Verify TypeScript 5.9 compatibility with Angular 21.This was previously flagged regarding Angular 20. Please verify that Angular 21.1.x officially supports TypeScript 5.9.x before merging.
Angular 21 TypeScript version support requirements
🧹 Nitpick comments (14)
stories/scrolling/basic.component.ts (1)
14-39: Consider using@elseinstead of a separate@if (!enabled)block.The two
@ifblocks can be simplified using the@if ...@else`` syntax, which is more idiomatic and avoids redundant condition evaluation.♻️ Suggested refactor
`@if` (enabled) { <aui-virtual-scroll-viewport itemSize="50" class="example-viewport" > <div *auiVirtualFor="let item of items" class="example-item" > {{ item }} </div> </aui-virtual-scroll-viewport> - } - `@if` (!enabled) { + } `@else` { <div class="example-viewport" > `@for` (item of items; track item) { <div class="example-item" > {{ item }} </div> } </div> }src/notification/notification.component.html (1)
32-45: Consider using@elsefor mutually exclusive conditions.These two conditions (
duration <= 0 || isHoverandduration > 0 && !isHover) are mutually exclusive. Using@if/@elsewould be slightly more concise and make the mutual exclusivity explicit:♻️ Optional refactor
- `@if` (duration <= 0 || isHover) { - <aui-icon - [class]="bem.element('close')" - icon="xmark" - (click)="close()" - ></aui-icon> - } - `@if` (duration > 0 && !isHover) { + `@if` (duration <= 0 || isHover) { + <aui-icon + [class]="bem.element('close')" + icon="xmark" + (click)="close()" + ></aui-icon> + } `@else` { <div [class]="bem.element('duration')" > {{ remains > 0 ? remains : 1 }}s </div> - } + }src/tree-select/tree-node.component.html (1)
12-29: Consider using@elsefor mutually exclusive conditions.The two
@ifblocks checkingnodeData.loadingand!nodeData.loadingare mutually exclusive. Using@if/@elsewould be more idiomatic and slightly more efficient.♻️ Suggested refactor using `@if/`@else
- `@if` (nodeData.loading) { - <aui-icon - class="aui-tree-node__loading" - margin="left" - size="16" - [icon]="'spinner'" - ></aui-icon> - } - `@if` (!nodeData.loading) { - <aui-icon - class="aui-tree-node__indicator" - [class.isVisible]="nodeData.children" - margin="left" - size="16" - [icon]="nodeData.expanded ? 'angle_down' : 'angle_right'" - (click)="switchExpanded()" - ></aui-icon> - } + `@if` (nodeData.loading) { + <aui-icon + class="aui-tree-node__loading" + margin="left" + size="16" + [icon]="'spinner'" + ></aui-icon> + } `@else` { + <aui-icon + class="aui-tree-node__indicator" + [class.isVisible]="nodeData.children" + margin="left" + size="16" + [icon]="nodeData.expanded ? 'angle_down' : 'angle_right'" + (click)="switchExpanded()" + ></aui-icon> + }src/date-picker/calendar/range-picker-panel/template.html (2)
49-99: Redundant@if (showTime)check.The inner
@if (showTime)at line 57 is unnecessary since the outer condition at line 49 already ensuresshowTimeis true. You can remove the inner@ifblock and keep only its content.♻️ Suggested simplification
`@if` (showFooter && showTime) { <aui-calendar-footer [class]="bem.element('footer')" [clearable]="clearable" [clearText]="clearText" (clear)="clear.next()" (confirm)="confirmValue(rangeValue)" > - `@if` (showTime) { <div [class]="bem.element('footer-content')" > <!-- ... content ... --> </div> - } </aui-calendar-footer> }
109-120: Use@elseinstead of separate@ifblocks.The
@if (!value)and@if (value)pattern should use@if/@elsefor idiomatic Angular control flow syntax. This is cleaner and ensures mutual exclusivity.♻️ Suggested refactor
- `@if` (!value) { - <span - class="placeholder date-holder" - >{{ placeholder }}</span - > - } - `@if` (value) { - <span - class="date-value date-holder" - >{{ value | date: FOOTER_DATE_FORMAT }}</span - > - } + `@if` (value) { + <span class="date-value date-holder">{{ value | date: FOOTER_DATE_FORMAT }}</span> + } `@else` { + <span class="placeholder date-holder">{{ placeholder }}</span> + }src/inline-alert/inline-alert.component.html (1)
6-10: Consider cachingiconMap(type)to avoid duplicate evaluation.
iconMap(type)is called twice in this template block. If this is a method rather than a getter or computed signal, consider using Angular's@letsyntax (available in Angular 18.1+) to store the result and avoid redundant computation during change detection.♻️ Suggested optimization using `@let`
+ `@let` icon = iconMap(type); <aui-icon class="aui-inline-alert__icon" - [icon]="iconMap(type)" - [background]="iconMap(type).endsWith('_triangle_s') ? 'triangle' : 'circle'" + [icon]="icon" + [background]="icon.endsWith('_triangle_s') ? 'triangle' : 'circle'" ></aui-icon>src/time-picker/panel/panel.template.html (2)
13-13: Inconsistent tracking expressions across@forloops.The three
@forloops use different tracking approaches:
- Hour (line 13):
track item- Minute (lines 36-38):
track trackBy($index, item)- Second (lines 61-64):
track itemSince
trackBy()simply returns the item value, all three should usetrack itemfor consistency. This also eliminates the unnecessary function call overhead.♻️ Suggested fix for consistent tracking
- `@for` ( - item of getControlTypeConfig(TimePickerControlType.Minute); track trackBy($index, - item)) { + `@for` (item of getControlTypeConfig(TimePickerControlType.Minute); track item) {- `@for` ( - item of getControlTypeConfig(TimePickerControlType.Second) - ; track - item) { + `@for` (item of getControlTypeConfig(TimePickerControlType.Second); track item) {Also applies to: 36-38, 61-64
82-101: Footer logic is correct; consider usingemit()instead ofnext().The
@if/@elsestructure for the footer template is well-implemented. Minor note: line 94 usesconfirm.next()whereasconfirm.emit()is the idiomatic Angular EventEmitter API. Both work sinceEventEmitterextendsSubject, butemit()better communicates intent.♻️ Optional: use idiomatic emit()
- (click)="$event.stopImmediatePropagation(); confirm.next()" + (click)="$event.stopImmediatePropagation(); confirm.emit()"stories/sort/sort.component.ts (1)
37-43: Consider tracking by unique identifier instead of object reference.Using
track itemtracks by object reference. For better change detection performance, especially if the data source is ever replaced or modified, tracking by a unique property is preferable:- `@for` (item of dataSource; track item) { + `@for` (item of dataSource; track item.id) {This is a demo component, so the current approach works, but it's a good practice to follow consistently.
src/anchor/anchor-tree.component.html (1)
11-17: Use aliasedlabelvariable consistently.On line 16,
{{ item.label }}should use the aliasedlabelvariable for consistency with the@ifcondition:`@if` (isTemplateRef(label)) { <ng-container *ngTemplateOutlet="label; context: item.labelContext" ></ng-container> } `@else` { - {{ item.label }} + {{ label }} }This maintains consistency and ensures the same value reference is used throughout the block.
src/breadcrumb/breadcrumb-item.component.html (1)
4-11: Consider using@if/@elsefor mutually exclusive conditions.The two conditions are mutually exclusive. Using
@elsewould be cleaner and more efficient (single condition evaluation).♻️ Suggested refactor
- `@if` (!separatorIcon) { - {{ separator }} - } - `@if` (separatorIcon) { - <aui-icon - [icon]="separatorIcon" - ></aui-icon> - } + `@if` (separatorIcon) { + <aui-icon [icon]="separatorIcon"></aui-icon> + } `@else` { + {{ separator }} + }src/select/select.component.html (1)
46-65: Multiple async pipe subscriptions on the same observable.Lines 46, 53, and 57 all evaluate
selectedOption$ | asyncseparately, creating multiple subscriptions to the same observable. While this works, it's slightly inefficient.Consider using a single
@ifwith an alias at the outer level to avoid redundant subscriptions:Suggested optimization
- `@if` ((selectedOption$ | async) && !filterString) { + `@if` (selectedOption$ | async; as selectedOption) { + `@if` (selectedOption && !filterString) { <div class="aui-select__label-container aui-input aui-input--{{ size }}" [attr.disabled]="disabled ? true : null" [attr.readonly]="readonly ? true : null" > <div class="aui-select__label"> - `@if` ((selectedOption$ | async).label; as optionLabel) { + `@if` (selectedOption.label; as optionLabel) { `@if` (isTemplateRef(optionLabel)) { <ng-container [ngTemplateOutlet]="optionLabel" - [ngTemplateOutletContext]="(selectedOption$ | async).labelContext" + [ngTemplateOutletContext]="selectedOption.labelContext" ></ng-container> } `@else` { {{ optionLabel }} } } </div> </div> + } }src/steps/steps.component.html (1)
14-63: Prefer@elsefor mutually exclusive progress branches.This avoids duplicate condition checks and makes the mutual exclusivity explicit.
♻️ Suggested refactor
- `@if` (isProgress) { - `@switch` (step.state) { - `@case` ('pending') { - <aui-icon - class="aui-step__indicator--pending" - [class.isActive]="i === activeIndex" - icon="rotate" - ></aui-icon> - } - `@case` ('done') { - <aui-icon - class="aui-step__indicator--done" - [class.isActive]="i === activeIndex" - icon="check" - ></aui-icon> - } - `@case` ('error') { - <aui-icon - class="aui-step__indicator--error" - [class.isActive]="i === activeIndex" - icon="xmark_small" - ></aui-icon> - } - `@default` { - <span - class="aui-step__indicator--index" - [class.isActive]="i === activeIndex" - > - {{ i + 1 }} - </span> - } - } - } - `@if` (!isProgress) { - `@if` (i >= currentIndex) { - <span - class="aui-step__indicator--index" - [class.isActive]="currentIndex === i" - > - {{ i + 1 }} - </span> - } - `@if` (i < currentIndex) { - <aui-icon - class="aui-step__indicator--done" - [class.isActive]="_currentIndex === i" - icon="check" - ></aui-icon> - } - } + `@if` (isProgress) { + `@switch` (step.state) { + `@case` ('pending') { + <aui-icon + class="aui-step__indicator--pending" + [class.isActive]="i === activeIndex" + icon="rotate" + ></aui-icon> + } + `@case` ('done') { + <aui-icon + class="aui-step__indicator--done" + [class.isActive]="i === activeIndex" + icon="check" + ></aui-icon> + } + `@case` ('error') { + <aui-icon + class="aui-step__indicator--error" + [class.isActive]="i === activeIndex" + icon="xmark_small" + ></aui-icon> + } + `@default` { + <span + class="aui-step__indicator--index" + [class.isActive]="i === activeIndex" + > + {{ i + 1 }} + </span> + } + } + } `@else` { + `@if` (i >= currentIndex) { + <span + class="aui-step__indicator--index" + [class.isActive]="currentIndex === i" + > + {{ i + 1 }} + </span> + } + `@if` (i < currentIndex) { + <aui-icon + class="aui-step__indicator--done" + [class.isActive]="_currentIndex === i" + icon="check" + ></aui-icon> + } + }src/tooltip/tooltip.component.html (1)
12-16: Avoid double subscription totemplate$async pipe.
template$ | asyncis evaluated in the condition and again inngTemplateOutlet; alias once and reuse to avoid multiple subscriptions.♻️ Suggested refactor
- `@if` (template$ | async) { + `@if` (template$ | async; as template) { <ng-container - [ngTemplateOutlet]="template$ | async" + [ngTemplateOutlet]="template" [ngTemplateOutletContext]="context$ | async" ></ng-container> }
yangxiaolang
force-pushed
the
chore/update-ng-20
branch
from
f85b4a7 to
3f0f954
Compare
yangxiaolang
force-pushed
the
chore/update-ng-20
branch
from
3353715 to
b929fca
Compare
yangxiaolang
force-pushed
the
chore/update-ng-20
branch
from
b929fca to
9f5395d
Compare
2 participants
Summary by CodeRabbit
New Features
Dependencies
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.