feat(modal): completed modal tedi-ready development #223#377
Merged
Conversation
added scroll-fade helper
mart-sessman
temporarily deployed
to
github-pages
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds ScrollFadeComponent (component, styles, stories, tests), introduces a CDK-backed ModalService with ModalRef and modal types, updates ModalComponent and subcomponents for service-mode compatibility and styling, and adjusts Jest and Stylelint configs (custom jsdom env; allow Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant ModalService as ModalService
participant CDK_Dialog as CDK_Dialog
participant DialogRef as DialogRef
participant ModalRef as ModalRef
participant ModalContent as ModalContent
Client->>ModalService: open(ContentComponent, config)
ModalService->>ModalService: build panel classes & position strategy
ModalService->>CDK_Dialog: open(component, dialogConfig)
CDK_Dialog-->>DialogRef: create DialogRef
ModalService->>ModalRef: new ModalRef(DialogRef)
ModalService->>ModalService: setupDialogBehavior(ref, overlayEl)
ModalService-->>Client: return ModalRef
Client->>ModalContent: receives injected ModalRef / MODAL_DATA
ModalContent->>ModalRef: close(result?)
ModalRef->>DialogRef: close(result)
DialogRef-->>Client: emit closed(result)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Stylelint (17.6.0)tedi/components/overlay/modal/modal.component.scssConfigurationError: Could not find "stylelint-config-recess-order". Do you need to install the package or use the "configBasedir" option? Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mart-sessman
temporarily deployed
to
github-pages
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
jest-jsdom-env.ts (1)
4-24: LGTM! Clean implementation for suppressing known jsdom CSS parsing limitations.The error filtering is appropriately narrow (exact message match on Error instances only), and the binding of
originalErrorbefore wrapping is correct. Consider adding a TODO to track removal once jsdom ≥22 is adopted:/** * Custom jest environment that suppresses jsdom "Could not parse CSS stylesheet" * errors. These occur because jsdom <22 does not support CSS `@layer` rules used * by Angular CDK overlay styles. + * + * TODO: Remove this workaround when upgrading to jsdom >=22 */,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jest-jsdom-env.ts` around lines 4 - 24, Add a short TODO comment above the PatchedJsdomEnvironment or the context.console.error wrapper that explains why this suppression exists, references jsdom version >=22 as the condition for removal, and notes that the filter targets the "Could not parse CSS stylesheet" Error emitted by jsdom; update the comment to mention the symbols PatchedJsdomEnvironment and context.console.error so future maintainers know where to remove the workaround once jsdom ≥22 is adopted.tedi/components/helpers/scroll-fade/scroll-fade.component.scss (1)
26-44: WebKit-only custom scrollbar styling.The
::-webkit-scrollbarpseudo-elements only work in WebKit/Blink browsers (Chrome, Safari, Edge). Firefox users will see the default scrollbar whenscrollBar="custom"is set. If cross-browser consistency is desired, consider addingscrollbar-widthandscrollbar-colorproperties for Firefox support.♻️ Optional: Add Firefox scrollbar support
&--custom-scroll { + scrollbar-width: thin; + scrollbar-color: var(--general-border-primary) var(--general-surface-primary); + &::-webkit-scrollbar { width: 6px; background-color: var(--general-surface-primary); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tedi/components/helpers/scroll-fade/scroll-fade.component.scss` around lines 26 - 44, The WebKit-only scrollbar rules under the &--custom-scroll block produce no effect in Firefox; update the &--custom-scroll selector (alongside the existing ::-webkit-scrollbar rules) to add cross-browser Firefox properties: add scrollbar-width (e.g., thin) and scrollbar-color using the same CSS variables (e.g., --general-border-primary for the thumb and --general-surface-primary for the track) and ensure hover state mapping is reflected by choosing an appropriate color (e.g., --general-border-secondary) so Firefox shows a matching custom scrollbar when scrollBar="custom".tedi/components/helpers/scroll-fade/scroll-fade.component.ts (1)
86-99: Scroll events emit on every qualifying scroll, not just on state transitions.
scrolledToTopandscrolledToBottomemit every timeonScroll()is called while at the respective edge, rather than only when transitioning to that edge. If the user holds position at top/bottom while scrolling slightly, this could fire multiple times.If this is intentional (e.g., for debouncing downstream), this is fine. Otherwise, consider tracking previous state to emit only on transitions:
♻️ Optional: Emit only on state transitions
+ private readonly wasAtTop = signal(true); + private readonly wasAtBottom = signal(false); private updateFade(scrollTop: number, scrollHeight: number, clientHeight: number): void { const atTop = scrollTop === 0; const atBottom = Math.abs(scrollHeight - scrollTop - clientHeight) <= 1; - if (atTop) { + if (atTop && !this.wasAtTop()) { this.scrolledToTop.emit(); } - if (atBottom) { + if (atBottom && !this.wasAtBottom()) { this.scrolledToBottom.emit(); } + this.wasAtTop.set(atTop); + this.wasAtBottom.set(atBottom); this.fade.set({ top: !atTop, bottom: !atBottom }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tedi/components/helpers/scroll-fade/scroll-fade.component.ts` around lines 86 - 99, The updateFade method currently emits scrolledToTop and scrolledToBottom on every scroll while at the edge; change it to emit only on transitions by tracking previous edge state (e.g., add component properties like previousAtTop and previousAtBottom), compute atTop/atBottom in updateFade, emit scrolledToTop only when atTop && !previousAtTop and scrolledToBottom only when atBottom && !previousAtBottom, then update previousAtTop/previousAtBottom before returning; keep the existing this.fade.set({ top: !atTop, bottom: !atBottom }) behavior.tedi/components/overlay/modal/modal.service.ts (1)
67-70: TwoModalRefinstances are created for the same dialog.Line 68 creates a
ModalReffor injection into the modal content component, and line 83 creates anotherModalRefto return to the caller. Both wrap the samedialogRef, so they work correctly, but this is redundant object creation.♻️ Proposed fix — reuse the injected ModalRef
+ let modalRef: ModalRef<R>; + const dialogRef = this.dialog.open<R, D>(component, { data, panelClass: panelClasses, backdropClass: "tedi-modal-backdrop", hasBackdrop: true, disableClose: true, ariaLabel, ariaLabelledBy, ariaModal: true, autoFocus: "first-tabbable", restoreFocus: true, positionStrategy: this.buildPositionStrategy(position, scrollBehavior), scrollStrategy: this.overlay.scrollStrategies.block(), - providers: (ref) => [ - { provide: ModalRef, useValue: new ModalRef<R>(ref) }, + providers: (ref) => { + modalRef = new ModalRef<R>(ref); + return [ + { provide: ModalRef, useValue: modalRef }, { provide: MODAL_DATA, useValue: data }, - ], + ]; + }, }); if (!isPresetWidth) { dialogRef.overlayRef.overlayElement.style.width = width; } if (maxWidth) { dialogRef.overlayRef.overlayElement.style.setProperty("--_tedi-modal-max-width", maxWidth); } this.setupDialogBehavior(dialogRef, scrollBehavior, closeOnBackdropClick, closeOnEscape); - return new ModalRef<R>(dialogRef); + return modalRef!; }Also applies to: 83-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tedi/components/overlay/modal/modal.service.ts` around lines 67 - 70, Create a single ModalRef instance and reuse it instead of constructing two objects; specifically, instantiate const modalRef = new ModalRef<R>(dialogRef) (or new ModalRef<R>(ref) depending on the surrounding variable) once, use that modalRef in the providers array ({ provide: ModalRef, useValue: modalRef }) and return the same modalRef to the caller (replace the separate new ModalRef(...) at the return site). Ensure MODAL_DATA provider remains unchanged and that the same dialogRef is passed into the single ModalRef instance.tedi/components/overlay/modal/modal.component.scss (1)
240-242: Consolidate duplicate&--centerselector blocks.The
&--centerselector for.tedi-modal-dialogis defined twice: lines 240-242 and 267-279. While both apply correctly, consolidating them improves maintainability.♻️ Proposed consolidation
&--center { max-height: var(--_tedi-modal-max-height); - } - - &--left, - &--right { - // ... left/right styles ... - } - - &--left { - animation: tedi-modal-slide-left 200ms ease-out; - } - - &--right { - animation: tedi-modal-slide-right 200ms ease-out; - } - - &--center { + // cdk-dialog-container: third-party, can't add host class cdk-dialog-container { display: flex; flex-direction: column; max-height: var(--_tedi-modal-max-height); } .tedi-modal { max-height: var(--_tedi-modal-max-height); overflow: hidden; } }Also applies to: 267-279
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tedi/components/overlay/modal/modal.component.scss` around lines 240 - 242, There are two duplicate selector blocks for .tedi-modal-dialog &--center; consolidate them by merging the properties from both blocks into a single &--center rule (preserve max-height and any other declarations from the second block such as centering, padding, or transforms), replace the two separate blocks with that single consolidated block, and remove the redundant duplicate to improve maintainability while keeping all original styling behavior.tedi/components/overlay/modal/modal.types.ts (1)
24-25: Clarify thatshowCloseis passed viadata, not handled by the service.The
showCloseproperty inModalConfigisn't consumed byModalServicedirectly — it's intended to be passed throughdataand applied to<tedi-modal-header [showClose]="...">in the content component. Consider adding a note to the JSDoc to clarify this pattern.📝 Proposed documentation update
- /** Whether to show a close button in the header. `@default` true */ + /** Whether to show a close button in the header. Pass via `data` and apply to `<tedi-modal-header [showClose]>`. `@default` true */ showClose?: boolean;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tedi/components/overlay/modal/modal.types.ts` around lines 24 - 25, Update the JSDoc for ModalConfig.showClose to clarify its usage: state that showClose is not consumed by ModalService but is intended to be provided inside the config.data object and forwarded to the content component (e.g., applied to <tedi-modal-header [showClose]="data.showClose">). Mention the default true value and give an example phrase like "pass via data to content component; ModalService does not handle this property." Reference ModalConfig, showClose, ModalService and tedi-modal-header in the comment so readers know where the prop should be used.tedi/components/overlay/modal/modal.component.ts (1)
75-77: Width presets are duplicated across files.The preset widths
["xs", "sm", "md", "lg", "xl"]are defined here and also inmodal.service.ts(line 8 asWIDTH_PRESETS). Consider extracting tomodal.types.tsfor a single source of truth.♻️ Proposed refactor in modal.types.ts
// In modal.types.ts export const MODAL_WIDTH_PRESETS = ["xs", "sm", "md", "lg", "xl"] as const; export type ModalWidthPreset = (typeof MODAL_WIDTH_PRESETS)[number];Then import and use
MODAL_WIDTH_PRESETSin bothmodal.component.tsandmodal.service.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tedi/components/overlay/modal/modal.component.ts` around lines 75 - 77, Extract the duplicated width presets into a new shared type file: create modal.types.ts exporting a const MODAL_WIDTH_PRESETS = ["xs","sm","md","lg","xl"] as const and a ModalWidthPreset type from it, then replace the local array in modal.component.ts (used by computed isPresetWidth) and the WIDTH_PRESETS usage in modal.service.ts with imports of MODAL_WIDTH_PRESETS and the ModalWidthPreset type so both files reference the single source of truth.tedi/components/overlay/modal/modal.component.spec.ts (1)
42-52: Remove duplicate assertions.Lines 47-48 duplicate the assertions already made on lines 45-46 for
tedi-modal--smandtedi-modal--center.🧹 Proposed fix
it("should apply correct default classes", () => { const classes = hostEl.getAttribute("class")!; expect(classes).toContain("tedi-modal--default"); expect(classes).toContain("tedi-modal--sm"); expect(classes).toContain("tedi-modal--center"); - expect(classes).toContain("tedi-modal--sm"); - expect(classes).toContain("tedi-modal--center"); expect(classes).not.toContain("tedi-modal--top"); expect(classes).not.toContain("tedi-modal--open"); expect(classes).not.toContain("tedi-modal--service"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tedi/components/overlay/modal/modal.component.spec.ts` around lines 42 - 52, The test "should apply correct default classes" contains duplicate assertions for the same classes; remove the redundant expect calls for "tedi-modal--sm" and "tedi-modal--center" so each class is asserted only once. In the spec for the modal component (the it block referencing hostEl.getAttribute("class")), delete the duplicated expect lines and keep one expect per class (including the negative assertions for "tedi-modal--top", "tedi-modal--open", and "tedi-modal--service").tedi/components/overlay/modal/modal.stories.ts (1)
441-528:lastResultis assigned but never displayed.The
lastResultproperty is updated in allopen*methods but the template doesn't display it. Either remove the dead code or add UI to show the result.♻️ Option 1: Remove unused property
class ServiceDemoComponent { private readonly modalService = inject(ModalService); - lastResult: string | null = null; private readonly options = defaultOptions; openCenter() { const ref = this.modalService.open<string>(StoryModalContentComponent, { data: { title: "Center modal", options: this.options }, width: "md", }); - ref.closed.subscribe((r) => this.lastResult = r ?? "dismissed"); + ref.closed.subscribe((r) => console.log("Closed with:", r)); } // ... similar changes to other methods }♻️ Option 2: Display the result in the template
template: ` <div style="display: flex; gap: 16px; flex-wrap: wrap;"> <!-- buttons... --> </div> + `@if` (lastResult) { + <p style="margin-top: 16px;">Last result: {{ lastResult }}</p> + } `,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tedi/components/overlay/modal/modal.stories.ts` around lines 441 - 528, ServiceDemoComponent declares lastResult and updates it in each open* method but the template never uses it; either remove lastResult and the ref.closed.subscribe(...) assignments in methods (or stop assigning) to eliminate dead code, or add a visible binding in the component template that shows the result (for example render lastResult with interpolation or a small status element under the controls) so the value updated by ref.closed.subscribe(...) is actually displayed; locate ServiceDemoComponent, the lastResult field, and the openCenter/openTop/openRight/openLeft/openCustomMaxWidth/openNoBackdropClose/openNoCloseButton/openMobileFullscreen/openWithDescription methods to apply the chosen fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.stylelintrc.json:
- Around line 9-11: Update the validation message for the CSS class-name rule to
match the regex by listing all allowed prefixes and keeping the BEM examples;
specifically change the message string that starts with "Class selector must
start with 'tedi-'" to mention "tedi-, cdk-, ng-, or float-ui-" and preserve the
BEM examples (e.g., .tedi-button, .tedi-button__icon, .tedi-button--primary)
while still including the "%s" selector placeholder.
In `@tedi/components/overlay/modal/modal.service.spec.ts`:
- Around line 162-171: The test currently only asserts ref is truthy after
dispatching an Escape keydown; change it to verify the modal actually closed by
asserting the dialog container was removed and/or the modal ref emitted a
closed/afterClosed event: after creating ref via
service.open(TestModalContentComponent) and dispatching the KeyboardEvent on
document.querySelector("cdk-dialog-container"), replace the existing
expect(ref).toBeTruthy() and the manual ref.close() call with an assertion like
expect(document.querySelector("cdk-dialog-container")).toBeNull() and/or
subscribe/assert on ref.closed or ref.afterClosed() to ensure the modal closed
in response to Escape.
---
Nitpick comments:
In `@jest-jsdom-env.ts`:
- Around line 4-24: Add a short TODO comment above the PatchedJsdomEnvironment
or the context.console.error wrapper that explains why this suppression exists,
references jsdom version >=22 as the condition for removal, and notes that the
filter targets the "Could not parse CSS stylesheet" Error emitted by jsdom;
update the comment to mention the symbols PatchedJsdomEnvironment and
context.console.error so future maintainers know where to remove the workaround
once jsdom ≥22 is adopted.
In `@tedi/components/helpers/scroll-fade/scroll-fade.component.scss`:
- Around line 26-44: The WebKit-only scrollbar rules under the &--custom-scroll
block produce no effect in Firefox; update the &--custom-scroll selector
(alongside the existing ::-webkit-scrollbar rules) to add cross-browser Firefox
properties: add scrollbar-width (e.g., thin) and scrollbar-color using the same
CSS variables (e.g., --general-border-primary for the thumb and
--general-surface-primary for the track) and ensure hover state mapping is
reflected by choosing an appropriate color (e.g., --general-border-secondary) so
Firefox shows a matching custom scrollbar when scrollBar="custom".
In `@tedi/components/helpers/scroll-fade/scroll-fade.component.ts`:
- Around line 86-99: The updateFade method currently emits scrolledToTop and
scrolledToBottom on every scroll while at the edge; change it to emit only on
transitions by tracking previous edge state (e.g., add component properties like
previousAtTop and previousAtBottom), compute atTop/atBottom in updateFade, emit
scrolledToTop only when atTop && !previousAtTop and scrolledToBottom only when
atBottom && !previousAtBottom, then update previousAtTop/previousAtBottom before
returning; keep the existing this.fade.set({ top: !atTop, bottom: !atBottom })
behavior.
In `@tedi/components/overlay/modal/modal.component.scss`:
- Around line 240-242: There are two duplicate selector blocks for
.tedi-modal-dialog &--center; consolidate them by merging the properties from
both blocks into a single &--center rule (preserve max-height and any other
declarations from the second block such as centering, padding, or transforms),
replace the two separate blocks with that single consolidated block, and remove
the redundant duplicate to improve maintainability while keeping all original
styling behavior.
In `@tedi/components/overlay/modal/modal.component.spec.ts`:
- Around line 42-52: The test "should apply correct default classes" contains
duplicate assertions for the same classes; remove the redundant expect calls for
"tedi-modal--sm" and "tedi-modal--center" so each class is asserted only once.
In the spec for the modal component (the it block referencing
hostEl.getAttribute("class")), delete the duplicated expect lines and keep one
expect per class (including the negative assertions for "tedi-modal--top",
"tedi-modal--open", and "tedi-modal--service").
In `@tedi/components/overlay/modal/modal.component.ts`:
- Around line 75-77: Extract the duplicated width presets into a new shared type
file: create modal.types.ts exporting a const MODAL_WIDTH_PRESETS =
["xs","sm","md","lg","xl"] as const and a ModalWidthPreset type from it, then
replace the local array in modal.component.ts (used by computed isPresetWidth)
and the WIDTH_PRESETS usage in modal.service.ts with imports of
MODAL_WIDTH_PRESETS and the ModalWidthPreset type so both files reference the
single source of truth.
In `@tedi/components/overlay/modal/modal.service.ts`:
- Around line 67-70: Create a single ModalRef instance and reuse it instead of
constructing two objects; specifically, instantiate const modalRef = new
ModalRef<R>(dialogRef) (or new ModalRef<R>(ref) depending on the surrounding
variable) once, use that modalRef in the providers array ({ provide: ModalRef,
useValue: modalRef }) and return the same modalRef to the caller (replace the
separate new ModalRef(...) at the return site). Ensure MODAL_DATA provider
remains unchanged and that the same dialogRef is passed into the single ModalRef
instance.
In `@tedi/components/overlay/modal/modal.stories.ts`:
- Around line 441-528: ServiceDemoComponent declares lastResult and updates it
in each open* method but the template never uses it; either remove lastResult
and the ref.closed.subscribe(...) assignments in methods (or stop assigning) to
eliminate dead code, or add a visible binding in the component template that
shows the result (for example render lastResult with interpolation or a small
status element under the controls) so the value updated by
ref.closed.subscribe(...) is actually displayed; locate ServiceDemoComponent,
the lastResult field, and the
openCenter/openTop/openRight/openLeft/openCustomMaxWidth/openNoBackdropClose/openNoCloseButton/openMobileFullscreen/openWithDescription
methods to apply the chosen fix.
In `@tedi/components/overlay/modal/modal.types.ts`:
- Around line 24-25: Update the JSDoc for ModalConfig.showClose to clarify its
usage: state that showClose is not consumed by ModalService but is intended to
be provided inside the config.data object and forwarded to the content component
(e.g., applied to <tedi-modal-header [showClose]="data.showClose">). Mention the
default true value and give an example phrase like "pass via data to content
component; ModalService does not handle this property." Reference ModalConfig,
showClose, ModalService and tedi-modal-header in the comment so readers know
where the prop should be used.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ffdd4f6f-c761-45aa-839f-61ea22313d10
📒 Files selected for processing (25)
.stylelintrc.jsonjest-jsdom-env.tsjest.config.tstedi/components/helpers/index.tstedi/components/helpers/scroll-fade/index.tstedi/components/helpers/scroll-fade/scroll-fade.component.htmltedi/components/helpers/scroll-fade/scroll-fade.component.scsstedi/components/helpers/scroll-fade/scroll-fade.component.spec.tstedi/components/helpers/scroll-fade/scroll-fade.component.tstedi/components/helpers/scroll-fade/scroll-fade.stories.tstedi/components/overlay/modal/index.tstedi/components/overlay/modal/modal-content/modal-content.component.tstedi/components/overlay/modal/modal-footer/modal-footer.component.tstedi/components/overlay/modal/modal-header/modal-header.component.htmltedi/components/overlay/modal/modal-header/modal-header.component.spec.tstedi/components/overlay/modal/modal-header/modal-header.component.tstedi/components/overlay/modal/modal-ref.tstedi/components/overlay/modal/modal.component.htmltedi/components/overlay/modal/modal.component.scsstedi/components/overlay/modal/modal.component.spec.tstedi/components/overlay/modal/modal.component.tstedi/components/overlay/modal/modal.service.spec.tstedi/components/overlay/modal/modal.service.tstedi/components/overlay/modal/modal.stories.tstedi/components/overlay/modal/modal.types.ts
Comment on lines
+9
to
11
| "^((tedi|cdk)-[a-z][a-z0-9]*(?:-[a-z0-9]+)*(?:__[a-z][a-z0-9]*(?:-[a-z0-9]+)*)*(?:--[a-z][a-z0-9]+(?:-[a-z0-9]+)*)?|ng-[a-z]+(?:-[a-z]+)*|float-ui-[a-z]+(?:-[a-z]+)*)$", | ||
| { | ||
| "message": "Class selector must start with 'tedi-' prefix and follow BEM naming (e.g., .tedi-button, .tedi-button__icon, .tedi-button--primary). Selector: \"%s\"", |
There was a problem hiding this comment.
Align the validation message with the updated regex.
On Line 9 the pattern now allows cdk-, ng-, and float-ui-, but the error text on Line 11 still states only tedi- is valid. This will confuse contributors during lint failures.
Suggested patch
- "message": "Class selector must start with 'tedi-' prefix and follow BEM naming (e.g., .tedi-button, .tedi-button__icon, .tedi-button--primary). Selector: \"%s\"",
+ "message": "Class selector must start with one of: 'tedi-', 'cdk-', 'ng-', or 'float-ui-' and follow BEM naming where applicable (e.g., .tedi-button, .tedi-button__icon, .tedi-button--primary). Selector: \"%s\"",📝 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
| "^((tedi|cdk)-[a-z][a-z0-9]*(?:-[a-z0-9]+)*(?:__[a-z][a-z0-9]*(?:-[a-z0-9]+)*)*(?:--[a-z][a-z0-9]+(?:-[a-z0-9]+)*)?|ng-[a-z]+(?:-[a-z]+)*|float-ui-[a-z]+(?:-[a-z]+)*)$", | |
| { | |
| "message": "Class selector must start with 'tedi-' prefix and follow BEM naming (e.g., .tedi-button, .tedi-button__icon, .tedi-button--primary). Selector: \"%s\"", | |
| "^((tedi|cdk)-[a-z][a-z0-9]*(?:-[a-z0-9]+)*(?:__[a-z][a-z0-9]*(?:-[a-z0-9]+)*)*(?:--[a-z][a-z0-9]+(?:-[a-z0-9]+)*)?|ng-[a-z]+(?:-[a-z]+)*|float-ui-[a-z]+(?:-[a-z]+)*)$", | |
| { | |
| "message": "Class selector must start with one of: 'tedi-', 'cdk-', 'ng-', or 'float-ui-' and follow BEM naming where applicable (e.g., .tedi-button, .tedi-button__icon, .tedi-button--primary). Selector: \"%s\"", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.stylelintrc.json around lines 9 - 11, Update the validation message for the
CSS class-name rule to match the regex by listing all allowed prefixes and
keeping the BEM examples; specifically change the message string that starts
with "Class selector must start with 'tedi-'" to mention "tedi-, cdk-, ng-, or
float-ui-" and preserve the BEM examples (e.g., .tedi-button,
.tedi-button__icon, .tedi-button--primary) while still including the "%s"
selector placeholder.
Comment on lines
+162
to
+171
| it("should close on Escape by default", () => { | ||
| const ref = service.open(TestModalContentComponent); | ||
|
|
||
| const event = new KeyboardEvent("keydown", { key: "Escape" }); | ||
| document.querySelector("cdk-dialog-container")?.dispatchEvent(event); | ||
|
|
||
| // The dialog should be closing/closed | ||
| expect(ref).toBeTruthy(); | ||
| ref.close(); | ||
| }); |
There was a problem hiding this comment.
Test does not verify Escape key actually closes the modal.
The test dispatches an Escape keydown event but only asserts that ref is truthy (which is always true after open()). It should verify the modal was closed, similar to the closeOnEscape: false test which checks that the overlay pane still exists.
🧪 Proposed fix
it("should close on Escape by default", () => {
- const ref = service.open(TestModalContentComponent);
+ service.open(TestModalContentComponent);
const event = new KeyboardEvent("keydown", { key: "Escape" });
document.querySelector("cdk-dialog-container")?.dispatchEvent(event);
- // The dialog should be closing/closed
- expect(ref).toBeTruthy();
- ref.close();
+ // Modal should be closed - overlay pane should not exist
+ const overlayPane = document.querySelector(".cdk-overlay-pane");
+ expect(overlayPane).toBeFalsy();
});📝 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
| it("should close on Escape by default", () => { | |
| const ref = service.open(TestModalContentComponent); | |
| const event = new KeyboardEvent("keydown", { key: "Escape" }); | |
| document.querySelector("cdk-dialog-container")?.dispatchEvent(event); | |
| // The dialog should be closing/closed | |
| expect(ref).toBeTruthy(); | |
| ref.close(); | |
| }); | |
| it("should close on Escape by default", () => { | |
| service.open(TestModalContentComponent); | |
| const event = new KeyboardEvent("keydown", { key: "Escape" }); | |
| document.querySelector("cdk-dialog-container")?.dispatchEvent(event); | |
| // Modal should be closed - overlay pane should not exist | |
| const overlayPane = document.querySelector(".cdk-overlay-pane"); | |
| expect(overlayPane).toBeFalsy(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tedi/components/overlay/modal/modal.service.spec.ts` around lines 162 - 171,
The test currently only asserts ref is truthy after dispatching an Escape
keydown; change it to verify the modal actually closed by asserting the dialog
container was removed and/or the modal ref emitted a closed/afterClosed event:
after creating ref via service.open(TestModalContentComponent) and dispatching
the KeyboardEvent on document.querySelector("cdk-dialog-container"), replace the
existing expect(ref).toBeTruthy() and the manual ref.close() call with an
assertion like expect(document.querySelector("cdk-dialog-container")).toBeNull()
and/or subscribe/assert on ref.closed or ref.afterClosed() to ensure the modal
closed in response to Escape.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tedi/components/overlay/modal/modal.service.ts (1)
67-84: Two separateModalRefinstances are created for the same dialog.Line 68 creates a
ModalReffor dependency injection into the content component, while line 83 creates another instance to return to the caller. Both wrap the sameDialogRef, so functionality is preserved, but this is redundant and could cause confusion during debugging (e.g., comparing object references).♻️ Reuse the same ModalRef instance
+ let modalRef!: ModalRef<R>; + const dialogRef = this.dialog.open<R, D>(component, { data, panelClass: panelClasses, backdropClass: "tedi-modal-backdrop", hasBackdrop: true, disableClose: true, ariaLabel, ariaLabelledBy, ariaModal: true, autoFocus: "first-tabbable", restoreFocus: true, positionStrategy: this.buildPositionStrategy(position, scrollBehavior), scrollStrategy: this.overlay.scrollStrategies.block(), providers: (ref) => [ - { provide: ModalRef, useValue: new ModalRef<R>(ref) }, + { provide: ModalRef, useFactory: () => (modalRef = new ModalRef<R>(ref)) }, { provide: MODAL_DATA, useValue: data }, ], }); // ...existing setup code... - return new ModalRef<R>(dialogRef); + return modalRef;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tedi/components/overlay/modal/modal.service.ts` around lines 67 - 84, Create a single ModalRef instance and reuse it instead of constructing two different ones: instantiate const modalRef = new ModalRef<R>(dialogRef) once, pass that modalRef to the providers array (where ModalRef is provided to the content) and also return modalRef from the create/open function; keep the existing dialogRef usage and calls to setupDialogBehavior(dialogRef, scrollBehavior, closeOnBackdropClick, closeOnEscape) unchanged so behavior is preserved.tedi/components/overlay/modal/modal.component.scss (1)
240-279: Consider consolidating duplicate&--centerselector blocks.The
&--centerselector is defined twice: once at lines 240-242 settingmax-height, and again at lines 267-279 forcdk-dialog-containerand.tedi-modalstyles. While SCSS will compile these correctly, consolidating them improves maintainability.♻️ Optional: Consolidate into a single block
- &--center { - max-height: var(--_tedi-modal-max-height); - } - &--left, &--right { min-height: 100dvh; // ...existing code... } // ...&--left and &--right blocks... &--center { + max-height: var(--_tedi-modal-max-height); + // cdk-dialog-container: third-party, can't add host class cdk-dialog-container { display: flex; flex-direction: column; max-height: var(--_tedi-modal-max-height); } .tedi-modal { max-height: var(--_tedi-modal-max-height); overflow: hidden; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tedi/components/overlay/modal/modal.component.scss` around lines 240 - 279, The SCSS defines the &--center selector in two places; consolidate into a single &--center block so all center-specific rules (max-height, cdk-dialog-container { display/flex-direction/max-height }, and .tedi-modal { max-height/overflow }) live together; update the existing &--center occurrences by merging the max-height declaration at the first &--center with the later cdk-dialog-container and .tedi-modal rules to remove duplication while preserving the same selectors (&--center, cdk-dialog-container, .tedi-modal) and values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tedi/components/overlay/modal/modal.component.scss`:
- Around line 240-279: The SCSS defines the &--center selector in two places;
consolidate into a single &--center block so all center-specific rules
(max-height, cdk-dialog-container { display/flex-direction/max-height }, and
.tedi-modal { max-height/overflow }) live together; update the existing
&--center occurrences by merging the max-height declaration at the first
&--center with the later cdk-dialog-container and .tedi-modal rules to remove
duplication while preserving the same selectors (&--center,
cdk-dialog-container, .tedi-modal) and values.
In `@tedi/components/overlay/modal/modal.service.ts`:
- Around line 67-84: Create a single ModalRef instance and reuse it instead of
constructing two different ones: instantiate const modalRef = new
ModalRef<R>(dialogRef) once, pass that modalRef to the providers array (where
ModalRef is provided to the content) and also return modalRef from the
create/open function; keep the existing dialogRef usage and calls to
setupDialogBehavior(dialogRef, scrollBehavior, closeOnBackdropClick,
closeOnEscape) unchanged so behavior is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b85f5191-1886-45a8-958d-1473d53e33ff
📒 Files selected for processing (5)
tedi/components/overlay/modal/modal.component.scsstedi/components/overlay/modal/modal.service.spec.tstedi/components/overlay/modal/modal.service.tstedi/components/overlay/modal/modal.stories.tstedi/components/overlay/modal/modal.types.ts
✅ Files skipped from review due to trivial changes (1)
- tedi/components/overlay/modal/modal.types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tedi/components/overlay/modal/modal.service.spec.ts
airikej
approved these changes
Apr 8, 2026
mart-sessman
temporarily deployed
to
github-pages
GitHub Actions
Inactive
github-actions Bot
pushed a commit
that referenced
this pull request
Apr 29, 2026
# [6.3.0](angular-6.2.1...angular-6.3.0) (2026-04-29) ### Bug Fixes * **button:** fixed invalid padding being set for neutral buttons [#383](#383) ([#391](#391)) ([426d231](426d231)) * **general:** update core version [#418](#418) ([#419](#419)) ([aa5cef7](aa5cef7)) * **modal:** added custom class to modal close button [#409](#409) ([#411](#411)) ([e81fa01](e81fa01)) * **text-field:** fix circular dependency issue when using with reactive forms [#382](#382) ([#392](#392)) ([89cf3f0](89cf3f0)) * **text-field:** fix disabled state tracking [#421](#421) ([#422](#422)) ([39db9ca](39db9ca)) * **variables:** update core version, fix variable names [#400](#400) ([#408](#408)) ([05a3153](05a3153)) ### Features * **accordion:** add clickable and iconPosition props to community accordion header [#552](https://github.com/TEDI-Design-System/angular/issues/552) ([d4f3251](d4f3251)) * **checkbox,radio:** added formControl support to groups [#412](#412) ([#413](#413)) ([077be96](077be96)) * **checkbox:** added checkbox-card variant [#222](#222) ([#378](#378)) ([d04a7d6](d04a7d6)) * **icon:** added inherit ColorType [#114](#114) ([#389](#389)) ([37836f4](37836f4)) * **modal:** completed modal tedi-ready development [#223](#223) ([#377](#377)) ([31868b0](31868b0)) * **radio:** new TEDI-ready component [#7](#7) ([#381](#381)) ([37d21ac](37d21ac)) * **select:** added searchFn, improved dropdown behavior [#403](#403) ([#404](#404)) ([03b13d0](03b13d0)) * **select:** new tedi-ready component [#15](#15) ([#343](#343)) ([56af62c](56af62c))
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
2 participants
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.
added scroll-fade helper
Summary by CodeRabbit
New Features
Improvements
Tests
Chores