feat(date-field,calendar): calendar tedi-ready #6#443
Open
mart-sessman wants to merge 3 commits into
Open
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR implements a comprehensive calendar and date field selection system for the TEDI Design System. It adds a multi-view calendar (day/month/year), a reactive date field form control with locale-aware date utilities, keyboard navigation, and both popover and modal presentation modes, supported by extensive test coverage and Storybook stories. ChangesCalendar and DateField Feature Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
mart-sessman
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@tedi/components/content/calendar/calendar-day-grid/calendar-day-grid.component.scss`:
- Around line 30-32: The bottom-border rule using the selector
"&:not(:first-child)" in calendar-day-grid.component.scss never matches the
week-number header cell, so remove the ":not(:first-child)" pseudo-class from
that rule (or replace it with a selector that targets the week-number header
cell explicitly, e.g., the week-number cell class) so the border is applied to
the week-number header; update the rule around the existing selector where
"&:not(:first-child)" appears (the calendar-day-grid cell styling) to use "&" or
the specific week-number selector instead.
- Around line 187-192: Replace the direct element selector styling for
tedi-status-indicator with a BEM-style class: add a class (e.g.
"calendar-day-grid__status" or "tedi-status-indicator") to the
tedi-status-indicator element in the calendar-day-grid template, then update
calendar-day-grid.component.scss to target that class instead of the element
selector; ensure positioning rules (position, top, right, pointer-events) are
moved to the new class and remove the original tedi-status-indicator element
selector.
In
`@tedi/components/content/calendar/calendar-header/calendar-header.component.html`:
- Line 32: Replace the nonstandard property binding ariaHaspopup="listbox" on
the calendar header trigger buttons with an attribute binding so assistive tech
sees the ARIA attribute; use [attr.aria-haspopup]="'listbox'" (matching how
[attr.aria-label] is used) and apply this change to all three trigger buttons in
the template so each trigger sets the actual aria-haspopup HTML attribute.
In
`@tedi/components/content/calendar/calendar-header/calendar-header.component.scss`:
- Around line 18-20: The SCSS rule currently targets the Angular element
selector "tedi-icon" (&.tedi-button tedi-icon) which we should avoid; change the
selector to target a CSS class (e.g., &.tedi-button .tedi-icon) and then add
that class to the icon host in the CalendarHeaderComponent template where the
tedi-icon is used (update the template to <tedi-icon class="tedi-icon"> or
similar). Ensure you keep the same style (font-size: var(--icon-03)) and update
any other usages to use the new class instead of relying on the element
selector.
In
`@tedi/components/content/calendar/calendar-month-grid/calendar-month-grid.component.html`:
- Around line 10-20: The month buttons in the calendar template only set
aria-disabled but not the native disabled attribute, so disabled months can
still receive focus and clicks; update the button element in
calendar-month-grid.component.html to bind the native disabled attribute using
[disabled]="isDisabled(i)" alongside the existing aria bindings so that
isDisabled(i) prevents native interaction (this affects the button with class
tedi-calendar-month-grid__month and the click handler handleClick(i)).
In `@tedi/components/content/calendar/calendar.stories.ts`:
- Around line 27-57: The module-level mutation of globalThis.Date (symbols
FIXED_TODAY_MS, GLOBAL_MARK, globalScope.Date, MockDate) must be removed;
instead implement a Storybook decorator (e.g., mockTodayDecorator) that, for
each story render, temporarily replaces Date with a MockDate that uses
FIXED_TODAY_MS, calls the story, and then restores the original Date in a
finally block so the global runtime is never permanently mutated; update the
stories export to apply this decorator to calendar stories and remove the
top-level patch/guard code from the module.
In `@tedi/components/form/date-field/date-field.component.html`:
- Around line 50-53: The tedi-calendar usage in date-field.component.html
forwards availableDays but omits unavailableDays, allowing blocked dates to be
selectable; update the tedi-calendar element to forward the unavailableDays
input (e.g., add [unavailableDays]="unavailableDays()") alongside existing
bindings such as [availableDays]="availableDays()",
[disabledMatchers]="disabledMatchers()",
[shouldDisableMonth]="shouldDisableMonth()", and
[shouldDisableYear]="shouldDisableYear()" so the calendar respects upstream
unavailable constraints.
In `@tedi/components/form/date-field/date-field.component.ts`:
- Around line 348-351: The modal open path allows re-entrancy: in the block that
checks useModal() and calls openModal() (and the similar block around the
469-505 region), add a guard using overlayOpen() so you only call openModal()
when overlayOpen() is false; specifically, change the logic so it first checks
if (this.useModal() && !this.overlayOpen()) then call this.openModal() and
return, preventing duplicate opens while a modal is active.
In `@tedi/components/form/date-field/date-input/date-input.component.ts`:
- Around line 103-106: handleInput currently emits inputChange even when the
component is non-interactive; update handleInput(event: Event) to early-return
when the component is disabled or readOnly (use the same guard pattern as the
other handlers, e.g. if (this.disabled || this.readOnly) return;) so it does not
call this.inputChange.emit(target.value) for programmatic or blocked events;
keep the rest of the logic (casting target to HTMLInputElement and emitting)
unchanged.
In `@tedi/components/overlay/modal/modal.stories.ts`:
- Around line 433-437: The confirm() handler currently shows a success toast
(this.toastService.success) but leaves the modal open; update confirm() to also
close the modal after showing the toast by invoking the component's modal-close
API (e.g., call the story/component instance method that closes the modal such
as this.close() or this.modalRef.close(), whichever is defined in this story) so
the action flow completes and the modal is dismissed after confirmation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 84a21556-a6b2-4aaf-a936-e01cc6c6c2fc
📒 Files selected for processing (54)
.storybook/preview-head.htmltedi/components/content/calendar/calendar-day-grid/calendar-day-grid.component.htmltedi/components/content/calendar/calendar-day-grid/calendar-day-grid.component.scsstedi/components/content/calendar/calendar-day-grid/calendar-day-grid.component.spec.tstedi/components/content/calendar/calendar-day-grid/calendar-day-grid.component.tstedi/components/content/calendar/calendar-day-grid/index.tstedi/components/content/calendar/calendar-header/calendar-header.component.htmltedi/components/content/calendar/calendar-header/calendar-header.component.scsstedi/components/content/calendar/calendar-header/calendar-header.component.spec.tstedi/components/content/calendar/calendar-header/calendar-header.component.tstedi/components/content/calendar/calendar-header/index.tstedi/components/content/calendar/calendar-month-grid/calendar-month-grid.component.htmltedi/components/content/calendar/calendar-month-grid/calendar-month-grid.component.scsstedi/components/content/calendar/calendar-month-grid/calendar-month-grid.component.spec.tstedi/components/content/calendar/calendar-month-grid/calendar-month-grid.component.tstedi/components/content/calendar/calendar-month-grid/index.tstedi/components/content/calendar/calendar-year-grid/calendar-year-grid.component.htmltedi/components/content/calendar/calendar-year-grid/calendar-year-grid.component.scsstedi/components/content/calendar/calendar-year-grid/calendar-year-grid.component.spec.tstedi/components/content/calendar/calendar-year-grid/calendar-year-grid.component.tstedi/components/content/calendar/calendar-year-grid/index.tstedi/components/content/calendar/calendar.component.htmltedi/components/content/calendar/calendar.component.scsstedi/components/content/calendar/calendar.component.spec.tstedi/components/content/calendar/calendar.component.tstedi/components/content/calendar/calendar.stories.tstedi/components/content/calendar/index.tstedi/components/content/calendar/types.tstedi/components/content/index.tstedi/components/form/date-field/date-field-modal/date-field-modal.component.spec.tstedi/components/form/date-field/date-field-modal/date-field-modal.component.tstedi/components/form/date-field/date-field.component.htmltedi/components/form/date-field/date-field.component.scsstedi/components/form/date-field/date-field.component.spec.tstedi/components/form/date-field/date-field.component.tstedi/components/form/date-field/date-field.stories.tstedi/components/form/date-field/date-input/date-input.component.htmltedi/components/form/date-field/date-input/date-input.component.scsstedi/components/form/date-field/date-input/date-input.component.spec.tstedi/components/form/date-field/date-input/date-input.component.tstedi/components/form/date-field/date-input/index.tstedi/components/form/date-field/index.tstedi/components/form/date-picker/date-picker.stories.tstedi/components/form/form-field/form-field.component.htmltedi/components/form/index.tstedi/components/form/select/select.component.scsstedi/components/overlay/modal/modal.stories.tstedi/index.tstedi/services/translation/translations.tstedi/utils/date.util.spec.tstedi/utils/date.util.tstedi/utils/index.tstedi/utils/matchers.util.spec.tstedi/utils/matchers.util.ts
Comment on lines
+30
to
+32
| &:not(:first-child) { | ||
| border-bottom: var(--tedi-borders-01) solid var(--general-border-primary); | ||
| } |
There was a problem hiding this comment.
Week-number header border rule is unreachable in current structure.
&:not(:first-child) won’t match the week-number header cell here, so the bottom border is never applied. If the border is intended, remove the pseudo-class guard.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@tedi/components/content/calendar/calendar-day-grid/calendar-day-grid.component.scss`
around lines 30 - 32, The bottom-border rule using the selector
"&:not(:first-child)" in calendar-day-grid.component.scss never matches the
week-number header cell, so remove the ":not(:first-child)" pseudo-class from
that rule (or replace it with a selector that targets the week-number header
cell explicitly, e.g., the week-number cell class) so the border is applied to
the week-number header; update the rule around the existing selector where
"&:not(:first-child)" appears (the calendar-day-grid cell styling) to use "&" or
the specific week-number selector instead.
Comment on lines
+187
to
+192
| tedi-status-indicator { | ||
| position: absolute; | ||
| top: var(--tedi-borders-02); | ||
| right: 0; | ||
| pointer-events: none; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Replace direct Angular element selector styling with a BEM class.
Styling tedi-status-indicator directly violates the component SCSS rule; add a class on the element in the template and style that class here instead.
Proposed change
- tedi-status-indicator {
+ .tedi-calendar-day-grid__status-indicator {
position: absolute;
top: var(--tedi-borders-02);
right: 0;
pointer-events: none;
}- <tedi-status-indicator
+ <tedi-status-indicator
+ class="tedi-calendar-day-grid__status-indicator"
[type]="status.type"
size="sm"
[hasBorder]="true"
/>As per coding guidelines: "Do not style Angular element selectors directly — add CSS classes to component hosts and style those classes instead."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@tedi/components/content/calendar/calendar-day-grid/calendar-day-grid.component.scss`
around lines 187 - 192, Replace the direct element selector styling for
tedi-status-indicator with a BEM-style class: add a class (e.g.
"calendar-day-grid__status" or "tedi-status-indicator") to the
tedi-status-indicator element in the calendar-day-grid template, then update
calendar-day-grid.component.scss to target that class instead of the element
selector; ensure positioning rules (position, top, right, pointer-events) are
moved to the new class and remove the original tedi-status-indicator element
selector.
| <button | ||
| tedi-dropdown-trigger | ||
| type="button" | ||
| ariaHaspopup="listbox" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "calendar-header.component.html" | head -5Repository: TEDI-Design-System/angular
Length of output: 152
🏁 Script executed:
# Once we find the file, let's examine its content, especially around the mentioned lines
if [ -f "tedi/components/content/calendar/calendar-header/calendar-header.component.html" ]; then
wc -l "tedi/components/content/calendar/calendar-header/calendar-header.component.html"
fiRepository: TEDI-Design-System/angular
Length of output: 154
🏁 Script executed:
# Read the file to see the actual attributes
cat -n "tedi/components/content/calendar/calendar-header/calendar-header.component.html"Repository: TEDI-Design-System/angular
Length of output: 8165
Use the valid ARIA attribute binding syntax for popup triggers.
ariaHaspopup="listbox" should use the [attr.] binding to properly expose the ARIA attribute to assistive technology: [attr.aria-haspopup]="'listbox'". The current syntax creates a property binding instead of setting the HTML attribute, which won't reliably communicate listbox semantics to screen readers. This is inconsistent with other ARIA attributes in the template (e.g., [attr.aria-label]).
Proposed fix
- ariaHaspopup="listbox"
+ [attr.aria-haspopup]="'listbox'"Apply the same replacement to all three trigger buttons in this template (lines 32, 85, 138).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ariaHaspopup="listbox" | |
| [attr.aria-haspopup]="'listbox'" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@tedi/components/content/calendar/calendar-header/calendar-header.component.html`
at line 32, Replace the nonstandard property binding ariaHaspopup="listbox" on
the calendar header trigger buttons with an attribute binding so assistive tech
sees the ARIA attribute; use [attr.aria-haspopup]="'listbox'" (matching how
[attr.aria-label] is used) and apply this change to all three trigger buttons in
the template so each trigger sets the actual aria-haspopup HTML attribute.
Comment on lines
+18
to
+20
| &.tedi-button tedi-icon { | ||
| font-size: var(--icon-03); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Avoid styling Angular element selectors directly.
Please replace the tedi-icon selector with a class on the icon host and style that class instead.
As per coding guidelines, “Do not style Angular element selectors directly — add CSS classes to component hosts and style those classes instead.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@tedi/components/content/calendar/calendar-header/calendar-header.component.scss`
around lines 18 - 20, The SCSS rule currently targets the Angular element
selector "tedi-icon" (&.tedi-button tedi-icon) which we should avoid; change the
selector to target a CSS class (e.g., &.tedi-button .tedi-icon) and then add
that class to the icon host in the CalendarHeaderComponent template where the
tedi-icon is used (update the template to <tedi-icon class="tedi-icon"> or
similar). Ensure you keep the same style (font-size: var(--icon-03)) and update
any other usages to use the new class instead of relying on the element
selector.
Comment on lines
+10
to
+20
| <button | ||
| type="button" | ||
| class="tedi-calendar-month-grid__month" | ||
| [class.tedi-calendar-month-grid__month--selected]="isSelected(i)" | ||
| [class.tedi-calendar-month-grid__month--current]="isCurrent(i)" | ||
| [class.tedi-calendar-month-grid__month--disabled]="isDisabled(i)" | ||
| [attr.aria-label]="monthLongNames()[i]" | ||
| [attr.aria-selected]="isSelected(i) ? 'true' : null" | ||
| [attr.aria-disabled]="isDisabled(i) ? 'true' : null" | ||
| (click)="handleClick(i)" | ||
| > |
There was a problem hiding this comment.
Bind native disabled state on month buttons.
aria-disabled alone is not enough for native button behavior; add [disabled]="isDisabled(i)" to prevent accidental keyboard/mouse interaction on disabled months.
Proposed fix
<button
type="button"
class="tedi-calendar-month-grid__month"
[class.tedi-calendar-month-grid__month--selected]="isSelected(i)"
[class.tedi-calendar-month-grid__month--current]="isCurrent(i)"
[class.tedi-calendar-month-grid__month--disabled]="isDisabled(i)"
+ [disabled]="isDisabled(i)"
[attr.aria-label]="monthLongNames()[i]"
[attr.aria-selected]="isSelected(i) ? 'true' : null"
[attr.aria-disabled]="isDisabled(i) ? 'true' : null"
(click)="handleClick(i)"
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <button | |
| type="button" | |
| class="tedi-calendar-month-grid__month" | |
| [class.tedi-calendar-month-grid__month--selected]="isSelected(i)" | |
| [class.tedi-calendar-month-grid__month--current]="isCurrent(i)" | |
| [class.tedi-calendar-month-grid__month--disabled]="isDisabled(i)" | |
| [attr.aria-label]="monthLongNames()[i]" | |
| [attr.aria-selected]="isSelected(i) ? 'true' : null" | |
| [attr.aria-disabled]="isDisabled(i) ? 'true' : null" | |
| (click)="handleClick(i)" | |
| > | |
| <button | |
| type="button" | |
| class="tedi-calendar-month-grid__month" | |
| [class.tedi-calendar-month-grid__month--selected]="isSelected(i)" | |
| [class.tedi-calendar-month-grid__month--current]="isCurrent(i)" | |
| [class.tedi-calendar-month-grid__month--disabled]="isDisabled(i)" | |
| [disabled]="isDisabled(i)" | |
| [attr.aria-label]="monthLongNames()[i]" | |
| [attr.aria-selected]="isSelected(i) ? 'true' : null" | |
| [attr.aria-disabled]="isDisabled(i) ? 'true' : null" | |
| (click)="handleClick(i)" | |
| > |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@tedi/components/content/calendar/calendar-month-grid/calendar-month-grid.component.html`
around lines 10 - 20, The month buttons in the calendar template only set
aria-disabled but not the native disabled attribute, so disabled months can
still receive focus and clicks; update the button element in
calendar-month-grid.component.html to bind the native disabled attribute using
[disabled]="isDisabled(i)" alongside the existing aria bindings so that
isDisabled(i) prevents native interaction (this affects the button with class
tedi-calendar-month-grid__month and the click handler handleClick(i)).
Comment on lines
+27
to
+57
| // Lock "today" for Chromatic stability. Patches the global Date constructor | ||
| // for this stories module so the calendar's internal `new Date()` checks | ||
| // (today indicator, focusable-day fallback) resolve to a fixed reference. | ||
| // | ||
| // Guard against double-patching: Storybook HMR re-evaluates this module, and | ||
| // without the guard each reload would wrap the previous MockDate in another | ||
| // subclass — every `new Date()` would then traverse a growing prototype chain, | ||
| // pegging CPU once enough reloads have happened (especially in Docs view | ||
| // where many calendars instantiate at once). | ||
| const FIXED_TODAY_MS = new Date(2026, 4, 16).getTime(); | ||
| const GLOBAL_MARK = "__tediCalendarMockDate"; | ||
| const globalScope = globalThis as unknown as Record<string, unknown> & { | ||
| Date: typeof Date; | ||
| }; | ||
| if (!globalScope[GLOBAL_MARK]) { | ||
| const RealDate = Date; | ||
| class MockDate extends RealDate { | ||
| constructor(...args: unknown[]) { | ||
| if (args.length === 0) { | ||
| super(FIXED_TODAY_MS); | ||
| return; | ||
| } | ||
| super(...(args as ConstructorParameters<typeof Date>)); | ||
| } | ||
| static override now(): number { | ||
| return FIXED_TODAY_MS; | ||
| } | ||
| } | ||
| globalScope.Date = MockDate as typeof Date; | ||
| globalScope[GLOBAL_MARK] = true; | ||
| } |
There was a problem hiding this comment.
Avoid mutating globalThis.Date in a stories module.
This patch affects the whole Storybook runtime, so other stories can get a frozen “today” unintentionally and become order-dependent/non-deterministic.
Suggested direction
-// module-level global Date patch
+// Prefer injecting a "now" source into CalendarComponent (e.g., token/service)
+// and override it only in this story via moduleMetadata providers.
+// This keeps story determinism without global side effects.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tedi/components/content/calendar/calendar.stories.ts` around lines 27 - 57,
The module-level mutation of globalThis.Date (symbols FIXED_TODAY_MS,
GLOBAL_MARK, globalScope.Date, MockDate) must be removed; instead implement a
Storybook decorator (e.g., mockTodayDecorator) that, for each story render,
temporarily replaces Date with a MockDate that uses FIXED_TODAY_MS, calls the
story, and then restores the original Date in a finally block so the global
runtime is never permanently mutated; update the stories export to apply this
decorator to calendar stories and remove the top-level patch/guard code from the
module.
Comment on lines
+50
to
+53
| [disabledMatchers]="disabledMatchers()" | ||
| [availableDays]="availableDays()" | ||
| [shouldDisableMonth]="shouldDisableMonth()" | ||
| [shouldDisableYear]="shouldDisableYear()" |
There was a problem hiding this comment.
Forward unavailableDays into tedi-calendar to preserve disable/availability behavior.
DateField forwards availableDays, but not unavailableDays. This can make blocked dates selectable in popover mode even when unavailable constraints are configured upstream.
Proposed fix
<tedi-calendar
`#calendar`
[bordered]="false"
[value]="value()"
[currentMonth]="currentMonth()"
[mode]="mode()"
[selectionLevel]="selectionLevel()"
[localeCode]="localeCode()"
[showOutsideDays]="showOutsideDays()"
[numberOfMonths]="numberOfMonthsResolved()"
[monthYearSelectType]="monthYearSelectType()"
[required]="required()"
[disabledMatchers]="disabledMatchers()"
[availableDays]="availableDays()"
+ [unavailableDays]="unavailableDays()"
[shouldDisableMonth]="shouldDisableMonth()"
[shouldDisableYear]="shouldDisableYear()"
[inputDisabled]="fieldDisabled()"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| [disabledMatchers]="disabledMatchers()" | |
| [availableDays]="availableDays()" | |
| [shouldDisableMonth]="shouldDisableMonth()" | |
| [shouldDisableYear]="shouldDisableYear()" | |
| [disabledMatchers]="disabledMatchers()" | |
| [availableDays]="availableDays()" | |
| [unavailableDays]="unavailableDays()" | |
| [shouldDisableMonth]="shouldDisableMonth()" | |
| [shouldDisableYear]="shouldDisableYear()" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tedi/components/form/date-field/date-field.component.html` around lines 50 -
53, The tedi-calendar usage in date-field.component.html forwards availableDays
but omits unavailableDays, allowing blocked dates to be selectable; update the
tedi-calendar element to forward the unavailableDays input (e.g., add
[unavailableDays]="unavailableDays()") alongside existing bindings such as
[availableDays]="availableDays()", [disabledMatchers]="disabledMatchers()",
[shouldDisableMonth]="shouldDisableMonth()", and
[shouldDisableYear]="shouldDisableYear()" so the calendar respects upstream
unavailable constraints.
Comment on lines
+348
to
+351
| if (this.useModal()) { | ||
| this.openModal(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Prevent duplicate modal opens while one is already active.
In modal mode, repeated icon clicks can trigger openModal() again before the first modal closes, which risks stacked modals and duplicate close handling. Guard this path with overlayOpen().
Proposed fix
handleIconClick(): void {
if (this.fieldDisabled()) return;
if (!this.enableCalendarResolved()) return;
@@
if (this.useModal()) {
+ if (this.overlayOpen()) return;
this.openModal();
return;
}Also applies to: 469-505
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tedi/components/form/date-field/date-field.component.ts` around lines 348 -
351, The modal open path allows re-entrancy: in the block that checks useModal()
and calls openModal() (and the similar block around the 469-505 region), add a
guard using overlayOpen() so you only call openModal() when overlayOpen() is
false; specifically, change the logic so it first checks if (this.useModal() &&
!this.overlayOpen()) then call this.openModal() and return, preventing duplicate
opens while a modal is active.
Comment on lines
+103
to
+106
| handleInput(event: Event): void { | ||
| const target = event.target as HTMLInputElement; | ||
| this.inputChange.emit(target.value); | ||
| } |
There was a problem hiding this comment.
Guard handleInput when disabled/readOnly.
Line 103 currently emits inputChange even if the component is non-interactive (when events are dispatched programmatically). Add the same guard pattern used by the other handlers.
Suggested fix
handleInput(event: Event): void {
+ if (this.disabled() || this.readOnly()) return;
const target = event.target as HTMLInputElement;
this.inputChange.emit(target.value);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| handleInput(event: Event): void { | |
| const target = event.target as HTMLInputElement; | |
| this.inputChange.emit(target.value); | |
| } | |
| handleInput(event: Event): void { | |
| if (this.disabled() || this.readOnly()) return; | |
| const target = event.target as HTMLInputElement; | |
| this.inputChange.emit(target.value); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tedi/components/form/date-field/date-input/date-input.component.ts` around
lines 103 - 106, handleInput currently emits inputChange even when the component
is non-interactive; update handleInput(event: Event) to early-return when the
component is disabled or readOnly (use the same guard pattern as the other
handlers, e.g. if (this.disabled || this.readOnly) return;) so it does not call
this.inputChange.emit(target.value) for programmatic or blocked events; keep the
rest of the logic (casting target to HTMLInputElement and emitting) unchanged.
Comment on lines
+433
to
+437
| confirm() { | ||
| const date = this.selectedDate(); | ||
| const formatted = date ? formatDate(date) : "no date selected"; | ||
| this.toastService.success("Saved", `Selected date: ${formatted}`); | ||
| } |
There was a problem hiding this comment.
Close the modal on confirm to complete the action flow.
confirm() shows a success toast but leaves the modal open, which makes the primary action feel incomplete.
Patch
confirm() {
const date = this.selectedDate();
const formatted = date ? formatDate(date) : "no date selected";
this.toastService.success("Saved", `Selected date: ${formatted}`);
+ this.ref.close("confirm");
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tedi/components/overlay/modal/modal.stories.ts` around lines 433 - 437, The
confirm() handler currently shows a success toast (this.toastService.success)
but leaves the modal open; update confirm() to also close the modal after
showing the toast by invoking the component's modal-close API (e.g., call the
story/component instance method that closes the modal such as this.close() or
this.modalRef.close(), whichever is defined in this story) so the action flow
completes and the modal is dismissed after confirmation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation