feat(float-ui): replaced float-ui usage in relevant components #390#401
feat(float-ui): replaced float-ui usage in relevant components #390#401mart-sessman wants to merge 5 commits into
Conversation
BREAKING CHANGES: ngx-float-ui dependency removed, appendTo inputs removed from related components
|
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 migrates overlay components (dropdown, popover, tooltip) from the ngx-float-ui library to Angular CDK's overlay system. It removes the ngx-float-ui dependency, adds a new positioning utility module, updates component public APIs, and revises documentation to include a code review step with the Changes
Sequence Diagram(s)sequenceDiagram
participant Component as Dropdown Component
participant Origin as CdkOverlayOrigin<br/>(Trigger)
participant CDK as CDK Overlay<br/>Service
participant Panel as Overlay Panel<br/>(Content)
participant Util as overlay-position<br/>Utility
Component->>Origin: attach CdkOverlayOrigin
Component->>Util: toConnectedPositions(placement)
Util-->>Component: ConnectedPosition[]
Component->>Component: set isOpen() = true
Component->>CDK: [cdkConnectedOverlay]="isOpen()"
CDK->>CDK: compute overlay position
CDK->>Util: query positioning logic
Util-->>CDK: calculated offset
CDK->>Panel: render with position
Panel-->>Component: (positionChange) event
Component->>Util: update currentPlacement
Util-->>Component: resolved placement
Component->>Component: set isOpen() = false
CDK->>Panel: hide overlay
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@coderabbitui review |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tedi/components/overlay/popover/popover.component.ts (2)
147-179:⚠️ Potential issue | 🟠 MajorStore
lockScrollstate at open time to avoid stale overflow management on close.When the popover opens,
showPopover()readslockScroll()and conditionally locks scroll. When it closes,hidePopover()readslockScroll()again and conditionally unlocks—but if the signal changed tofalsewhile open, the unlock is skipped, leavingdocument.bodyoverflow permanently hidden. Instead, store the state at open time and restore it based on that state, not the current signal value. The same pattern applies tohideOnScrollanddismissible: if they change fromfalsetotruewhile the popover is open, the listeners won't be attached becauseonOverlayAttach()only runs once. Useeffect()to watch these inputs and re-run setup/cleanup as needed, or store initial state and restore it consistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tedi/components/overlay/popover/popover.component.ts` around lines 147 - 179, Store the boolean values used at open time instead of re-reading reactive signals on close and to handle dynamic inputs: in showPopover() capture and save this._lockedScrollAtOpen = this.lockScroll(), this._hideOnScrollAtOpen = this.hideOnScroll(), and this._dismissibleAtOpen = this.dismissible(); use _lockedScrollAtOpen in hidePopover() to decide whether to restore document.body overflow; in onOverlayAttach() use the captured _hideOnScrollAtOpen and _dismissibleAtOpen to decide whether to call setupScrollListener() and setupDismissListeners(), and add an effect() that watches lockScroll(), hideOnScroll(), and dismissible() to attach or detach listeners via setupScrollListener()/teardownScrollListener() and setupDismissListeners()/teardownDismissListeners() so listeners are reconciled while the popover is open (refer to showPopover(), hidePopover(), onOverlayAttach(), setupScrollListener(), setupDismissListeners()).
158-180:⚠️ Potential issue | 🟠 MajorFilter overlay descendants from next-focus resolution to prevent tabbing into detached overlay.
When
focusElementAfterTrigger()is called (lines 325–336), it scans the entire body for focusable elements while the popover overlay is still in the DOM. If the trigger element is at the end of the page's focusable list, the next element in the list could be a child of the popover. AfterhidePopover()removes the overlay, the queuedfocus()call targets a now-detached element, breaking keyboard navigation.Solution: Filter out overlay descendants from
getFocusableElements(this.document.body)before determining the next focus target, or resolve the next target afterhidePopover()completes. The same issue applies tofocusElementBeforeTrigger()(lines 338–349).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tedi/components/overlay/popover/popover.component.ts` around lines 158 - 180, The focus restoration routine can pick a focusable element inside the still-attached overlay and later call focus() on a detached node; update focusElementAfterTrigger() and focusElementBeforeTrigger() to exclude overlay descendants when computing getFocusableElements(this.document.body) (filter out elements contained in this.connectedOverlay()?.overlayRef?.overlayElement or the ".tedi-popover__container") before picking the next/previous target, or alternatively compute the target after hidePopover() completes; ensure hidePopover(), focusElementAfterTrigger(), and focusElementBeforeTrigger() are updated to use this filtered list so focus() never targets detached overlay elements.tedi/components/overlay/dropdown/dropdown.component.ts (1)
2-10:⚠️ Potential issue | 🟠 MajorWrap the self-provider in
forwardRef().Angular's DI and the project standards require
forwardRef()when a component references itself in its ownprovidersarray. The decorator metadata is evaluated before the class definition is fully available, so the reference must be deferred. The codebase consistently applies this pattern across all similar providers (form controls withNG_VALUE_ACCESSOR, and tokens likeDROPDOWN_CONTENT_API).Suggested fix
import { Component, input, + forwardRef, ViewEncapsulation, ChangeDetectionStrategy, contentChild, signal, computed, model, } from "@angular/core"; @@ providers: [ { provide: DROPDOWN_API, - useExisting: DropdownComponent, + useExisting: forwardRef(() => DropdownComponent), }, ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tedi/components/overlay/dropdown/dropdown.component.ts` around lines 2 - 10, The component's self-provider in dropdown.component.ts must be wrapped with Angular's forwardRef to defer the class reference; update the providers entry that references the component itself (e.g., where DropdownComponent is provided for NG_VALUE_ACCESSOR or DROPDOWN_CONTENT_API) to use forwardRef(() => DropdownComponent), and add import { forwardRef } from '@angular/core' if missing so the decorator metadata no longer references the class before it's defined.
♻️ Duplicate comments (1)
.claude/skills/contributing/references/refactoring.md (1)
43-46:⚠️ Potential issue | 🟡 MinorDocument the
/simplifycommand and clarify "valid findings".The new code review step introduces the
/simplifycommand without explanation. Users need to know:
- What the
/simplifycommand is and what it does- Where/how to execute it (CLI, IDE plugin, web interface, etc.)
- What kind of output to expect
- How to determine which findings are "valid" (criteria, judgment process)
Consider adding a brief explanation or a reference link to documentation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/contributing/references/refactoring.md around lines 43 - 46, Update "Step 5: Code Review" to document the /simplify command and define "valid findings": add a short description of what /simplify does and where it runs (CLI/IDE/web), show the exact invocation or UI steps to execute it, describe expected output format (types of findings, severity levels, examples), and provide clear criteria for marking findings as "valid" (e.g., correctness, performance, readability, duplication) plus a link to further docs or help; reference the Step 5 header and the literal "/simplify" command so reviewers can find and expand the current text.
🧹 Nitpick comments (3)
tedi/components/overlay/overlay-position.util.ts (1)
184-195: Narrow the placement type used by the arrow helpers.
calculateArrowOffset()only handles the four resolved sides, but both it andgetPlacementFromPositionChange()are typed asstring. That lets raw values like"auto"or"top-start"compile here, and those inputs fall into the wrong branch in the arrow math.♻️ Suggested typing cleanup
+type ResolvedOverlaySide = "top" | "bottom" | "left" | "right"; + export function getPlacementFromPositionChange( change: ConnectedOverlayPositionChange, -): string { +): ResolvedOverlaySide { export function calculateArrowOffset( - placement: string, + placement: ResolvedOverlaySide, triggerEl: HTMLElement, overlayEl: HTMLElement,Also applies to: 272-308
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tedi/components/overlay/overlay-position.util.ts` around lines 184 - 195, Change the loose string typing to a strict placement union so only the four resolved sides are allowed: update getPlacementFromPositionChange to return a union type ('top' | 'bottom' | 'left' | 'right') instead of string, and narrow any callers/related helpers (notably calculateArrowOffset) to accept that same placement union; ensure getPlacementFromPositionChange maps ConnectedOverlayPositionChange.connectionPair values into that union only and update the arrow helper signatures (and any other usages in the 272-308 range) to use the new placement type so invalid values like "auto" or "top-start" are rejected at compile time.tedi/components/layout/header/header-language/header-language.component.ts (1)
3-12: Use the signal query API for the popover reference.Since this handler is already being touched, migrating the popover query off
@ViewChildkeeps the file consistent with the repo’s signal-based component pattern and removes the last decorator-style query from this component.♻️ Suggested refactor
import { ChangeDetectionStrategy, Component, computed, - forwardRef, inject, input, output, - ViewChild, ViewEncapsulation, + viewChild, } from "@angular/core"; @@ - `@ViewChild`(forwardRef(() => PopoverComponent)) popover?: PopoverComponent; + popover = viewChild(PopoverComponent); @@ - this.popover?.hidePopover(); + this.popover()?.hidePopover();As per coding guidelines, "Use Angular signals API (
input(),model(),signal(),computed()) instead of@Input(),@Output(),@ViewChild()decorators".Also applies to: 60-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tedi/components/layout/header/header-language/header-language.component.ts` around lines 3 - 12, Replace the `@ViewChild` decorator-based popover query with a signal-based query: remove the ViewChild import and the `@ViewChild`(...) popover property, add a signal like popoverRef = signal<HTMLElement | null>(null) (or the appropriate component type), and ensure the template uses a template reference variable (e.g. `#popover`) and the component sets popoverRef (e.g. in ngAfterViewInit or via a small template binding/handler) so all usages that referenced the old popover property now read from popoverRef() (or computed() wrappers) — update any code that accessed the old popover symbol to use the new popoverRef signal.tedi/components/overlay/popover/popover.component.spec.ts (1)
205-210: Don't bypass the overlay's real attach lifecycle in these specs.
CdkConnectedOverlayalready exposes anattachoutput. Callingcomponent.onOverlayAttach()directly here means these tests will still pass if the template binding is removed or stops firing, so they no longer guard the real overlay lifecycle wiring. Prefer opening the popover and asserting the event-driven path, or keep one focused unit test foronOverlayAttach()itself. (next.material.angular.dev)Also applies to: 221-225, 235-237, 319-321, 394-396, 413-415
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tedi/components/overlay/popover/popover.component.spec.ts` around lines 205 - 210, Tests call component.onOverlayAttach() directly which bypasses the template-driven CdkConnectedOverlay.attach event; instead locate the CdkConnectedOverlay directive instance in the test (query the overlay directive from the fixture), and drive the lifecycle by emitting its attach output (e.g., overlayDirective.attach.emit()) or by opening the popover with component.showPopover() and asserting the attach emitter is fired; replace direct onOverlayAttach() calls with this event-driven emit in the affected specs (references: component.showPopover(), onOverlayAttach(), CdkConnectedOverlay.attach); keep one focused unit test that directly calls onOverlayAttach() only if you want to verify its internal behavior separately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/contributing/references/new-component.md:
- Around line 57-60: Update "Step 6: Code Review" to document the /simplify
command and define "valid findings": add a short description of what /simplify
does (e.g., automated code-review assistant that suggests reuse, quality, and
efficiency improvements), where to run it (CLI command or IDE/web integration)
and what output looks like (summary of issues, severity levels, suggested fixes,
and example output). Also add criteria for "valid findings" (e.g., reproducible
issues, non-whitespace changes, security/bug/performance regressions, and
clearly actionable suggestions) and a short guidance on how to triage
suggestions (accept, request changes, or ignore with reason); include a
reference link to the tool/docs if available and mention the command name
"/simplify" and the Step heading ("Step 6: Code Review") so reviewers can find
and run it.
In @.stylelintrc.json:
- Around line 9-11: The lint message for the class selector rule incorrectly
states selectors must start with 'tedi-' even though the regex pattern
"^((tedi|cdk)-...|ng-... )$" allows 'tedi-', 'cdk-' and 'ng-' prefixes; update
the message string (the second item paired with that regex) to list all allowed
prefixes and keep the BEM guidance (e.g., "Class selector must start with
'tedi-', 'cdk-' or 'ng-' prefix and follow BEM naming (e.g., .tedi-button,
.cdk-modal, .ng-component, .tedi-button__icon, .tedi-button--primary). Selector:
\"%s\"").
In
`@tedi/components/overlay/dropdown/dropdown-trigger/dropdown-trigger.directive.ts`:
- Around line 66-77: openAndFocusFirst and openAndFocusLast call showDropdown
then schedule an extra setTimeout-based focus (focusFirstItem/focusLastItem)
which conflicts with DropdownComponent.showDropdown's own queued
focusActiveItem; remove the competing timer and instead add an option to
DropdownComponent.showDropdown (e.g., showDropdown({ initialFocus?:
'auto'|'first'|'last' })) or a boolean flag to suppress its default post-open
focusActiveItem so the caller can request first/last focus without a second
deferred focus; update openAndFocusFirst/openAndFocusLast to call showDropdown
with the new option (and remove the setTimeout calls) or, alternatively, expose
a parameter to skip the internal focus and let these methods perform the single
focus pass.
In `@tedi/components/overlay/dropdown/dropdown.component.html`:
- Around line 3-7: The outside-click handler currently always refocuses the
trigger via onOutsideClick(), breaking pointer interaction; change the template
to pass the event to the handler (overlayOutsideClick)="onOutsideClick($event)"
and update dropdown.component.ts onOutsideClick(event) to only refocus the
trigger when the dismissal is from keyboard (e.g., event is a KeyboardEvent or
event.key === 'Escape') and for pointer-driven events (MouseEvent) simply close
the menu without moving focus; keep existing close logic and ensure any
keyboard-driven escape path still calls the refocus code so keyboard dismissals
behave unchanged.
In `@tedi/components/overlay/dropdown/dropdown.component.spec.ts`:
- Around line 69-72: The afterEach teardown currently calls
dropdown.hideDropdown() and clears overlayContainerElement.innerHTML but does
not restore Jest timers; add a call to jest.useRealTimers() in the same
afterEach to ensure any tests that switched to fake timers (e.g., those using
fakeAsync/tick) cannot leak a fake clock into later tests — update the afterEach
block that invokes dropdown.hideDropdown() and overlayContainerElement.innerHTML
to also call jest.useRealTimers().
In `@tedi/components/overlay/dropdown/dropdown.component.ts`:
- Around line 103-106: The onOutsideClick handler currently calls
this.hideDropdown() and then unconditionally focuses the trigger element, which
reverts focus when the user clicks another control; remove the call to
this.dropdownTrigger().host.nativeElement.focus() from onOutsideClick so outside
(pointer) clicks simply close the overlay without restoring focus, and leave
keyboard dismissal/focus restoration to the existing Escape/trigger directive
logic (verify the escape handler in the trigger directive retains its focus
behavior). Ensure no other callers rely on onOutsideClick to restore focus and
update any related tests to reflect pointer-driven outside clicks should not
move focus.
In `@tedi/components/overlay/tooltip/tooltip.component.ts`:
- Around line 108-113: The showTooltip() implementation must always cancel any
pending hide timer before attempting to re-open; move or perform
clearTimeout(this.hideTimeout) unconditionally at the start of showTooltip()
(before or independent of the this.isOpen() check) so that an armed hideTimeout
cannot remove the tooltip after re-showing; update the method that references
showTooltip, this.hideTimeout, and this.isOpen to ensure the timer is cleared
every time showTooltip is called.
---
Outside diff comments:
In `@tedi/components/overlay/dropdown/dropdown.component.ts`:
- Around line 2-10: The component's self-provider in dropdown.component.ts must
be wrapped with Angular's forwardRef to defer the class reference; update the
providers entry that references the component itself (e.g., where
DropdownComponent is provided for NG_VALUE_ACCESSOR or DROPDOWN_CONTENT_API) to
use forwardRef(() => DropdownComponent), and add import { forwardRef } from
'@angular/core' if missing so the decorator metadata no longer references the
class before it's defined.
In `@tedi/components/overlay/popover/popover.component.ts`:
- Around line 147-179: Store the boolean values used at open time instead of
re-reading reactive signals on close and to handle dynamic inputs: in
showPopover() capture and save this._lockedScrollAtOpen = this.lockScroll(),
this._hideOnScrollAtOpen = this.hideOnScroll(), and this._dismissibleAtOpen =
this.dismissible(); use _lockedScrollAtOpen in hidePopover() to decide whether
to restore document.body overflow; in onOverlayAttach() use the captured
_hideOnScrollAtOpen and _dismissibleAtOpen to decide whether to call
setupScrollListener() and setupDismissListeners(), and add an effect() that
watches lockScroll(), hideOnScroll(), and dismissible() to attach or detach
listeners via setupScrollListener()/teardownScrollListener() and
setupDismissListeners()/teardownDismissListeners() so listeners are reconciled
while the popover is open (refer to showPopover(), hidePopover(),
onOverlayAttach(), setupScrollListener(), setupDismissListeners()).
- Around line 158-180: The focus restoration routine can pick a focusable
element inside the still-attached overlay and later call focus() on a detached
node; update focusElementAfterTrigger() and focusElementBeforeTrigger() to
exclude overlay descendants when computing
getFocusableElements(this.document.body) (filter out elements contained in
this.connectedOverlay()?.overlayRef?.overlayElement or the
".tedi-popover__container") before picking the next/previous target, or
alternatively compute the target after hidePopover() completes; ensure
hidePopover(), focusElementAfterTrigger(), and focusElementBeforeTrigger() are
updated to use this filtered list so focus() never targets detached overlay
elements.
---
Duplicate comments:
In @.claude/skills/contributing/references/refactoring.md:
- Around line 43-46: Update "Step 5: Code Review" to document the /simplify
command and define "valid findings": add a short description of what /simplify
does and where it runs (CLI/IDE/web), show the exact invocation or UI steps to
execute it, describe expected output format (types of findings, severity levels,
examples), and provide clear criteria for marking findings as "valid" (e.g.,
correctness, performance, readability, duplication) plus a link to further docs
or help; reference the Step 5 header and the literal "/simplify" command so
reviewers can find and expand the current text.
---
Nitpick comments:
In `@tedi/components/layout/header/header-language/header-language.component.ts`:
- Around line 3-12: Replace the `@ViewChild` decorator-based popover query with a
signal-based query: remove the ViewChild import and the `@ViewChild`(...) popover
property, add a signal like popoverRef = signal<HTMLElement | null>(null) (or
the appropriate component type), and ensure the template uses a template
reference variable (e.g. `#popover`) and the component sets popoverRef (e.g. in
ngAfterViewInit or via a small template binding/handler) so all usages that
referenced the old popover property now read from popoverRef() (or computed()
wrappers) — update any code that accessed the old popover symbol to use the new
popoverRef signal.
In `@tedi/components/overlay/overlay-position.util.ts`:
- Around line 184-195: Change the loose string typing to a strict placement
union so only the four resolved sides are allowed: update
getPlacementFromPositionChange to return a union type ('top' | 'bottom' | 'left'
| 'right') instead of string, and narrow any callers/related helpers (notably
calculateArrowOffset) to accept that same placement union; ensure
getPlacementFromPositionChange maps
ConnectedOverlayPositionChange.connectionPair values into that union only and
update the arrow helper signatures (and any other usages in the 272-308 range)
to use the new placement type so invalid values like "auto" or "top-start" are
rejected at compile time.
In `@tedi/components/overlay/popover/popover.component.spec.ts`:
- Around line 205-210: Tests call component.onOverlayAttach() directly which
bypasses the template-driven CdkConnectedOverlay.attach event; instead locate
the CdkConnectedOverlay directive instance in the test (query the overlay
directive from the fixture), and drive the lifecycle by emitting its attach
output (e.g., overlayDirective.attach.emit()) or by opening the popover with
component.showPopover() and asserting the attach emitter is fired; replace
direct onOverlayAttach() calls with this event-driven emit in the affected specs
(references: component.showPopover(), onOverlayAttach(),
CdkConnectedOverlay.attach); keep one focused unit test that directly calls
onOverlayAttach() only if you want to verify its internal behavior separately.
🪄 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: b73c4d35-8d02-4c78-b978-d4dc7b026c9e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (40)
.claude/skills/contributing/references/new-component.md.claude/skills/contributing/references/refactoring.md.github/workflows/angular-test-and-lint.yml.stylelintrc.jsonREADME.mdpackage.jsonskills/tedi-angular/SKILL.mdskills/tedi-angular/references/components.mdtedi/components/form/date-picker/date-picker.component.htmltedi/components/form/date-picker/date-picker.component.spec.tstedi/components/form/date-picker/date-picker.component.tstedi/components/form/filter/filter.component.htmltedi/components/form/filter/filter.component.spec.tstedi/components/form/filter/filter.component.tstedi/components/form/filter/filter.stories.tstedi/components/layout/header/header-language/header-language.component.htmltedi/components/layout/header/header-language/header-language.component.tstedi/components/layout/header/header-profile/header-profile.component.htmltedi/components/layout/header/header-role/header-role.component.htmltedi/components/overlay/dropdown/dropdown-trigger/dropdown-trigger.directive.tstedi/components/overlay/dropdown/dropdown.component.htmltedi/components/overlay/dropdown/dropdown.component.scsstedi/components/overlay/dropdown/dropdown.component.spec.tstedi/components/overlay/dropdown/dropdown.component.tstedi/components/overlay/dropdown/dropdown.stories.tstedi/components/overlay/dropdown/dropdown.tokens.tstedi/components/overlay/index.tstedi/components/overlay/overlay-position.util.tstedi/components/overlay/popover/popover-content/popover-content.component.spec.tstedi/components/overlay/popover/popover-trigger/popover-trigger.directive.tstedi/components/overlay/popover/popover.component.htmltedi/components/overlay/popover/popover.component.scsstedi/components/overlay/popover/popover.component.spec.tstedi/components/overlay/popover/popover.component.tstedi/components/overlay/tooltip/tooltip-trigger/tooltip-trigger.component.tstedi/components/overlay/tooltip/tooltip.component.htmltedi/components/overlay/tooltip/tooltip.component.scsstedi/components/overlay/tooltip/tooltip.component.spec.tstedi/components/overlay/tooltip/tooltip.component.tstedi/components/overlay/tooltip/tooltip.stories.ts
💤 Files with no reviewable changes (2)
- skills/tedi-angular/SKILL.md
- tedi/components/overlay/popover/popover-content/popover-content.component.spec.ts
| ## Step 6: Code Review | ||
|
|
||
| Run `/simplify` to review all changed code for reuse opportunities, code quality issues, and efficiency problems. Fix all valid findings before proceeding. | ||
|
|
There was a problem hiding this comment.
Document the /simplify command and clarify "valid findings".
The new code review step introduces the /simplify command without explanation. Users need to know:
- What the
/simplifycommand is and what it does - Where/how to execute it (CLI, IDE plugin, web interface, etc.)
- What kind of output to expect
- How to determine which findings are "valid" (criteria, judgment process)
Consider adding a brief explanation or a reference link to documentation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/contributing/references/new-component.md around lines 57 -
60, Update "Step 6: Code Review" to document the /simplify command and define
"valid findings": add a short description of what /simplify does (e.g.,
automated code-review assistant that suggests reuse, quality, and efficiency
improvements), where to run it (CLI command or IDE/web integration) and what
output looks like (summary of issues, severity levels, suggested fixes, and
example output). Also add criteria for "valid findings" (e.g., reproducible
issues, non-whitespace changes, security/bug/performance regressions, and
clearly actionable suggestions) and a short guidance on how to triage
suggestions (accept, request changes, or ignore with reason); include a
reference link to the tool/docs if available and mention the command name
"/simplify" and the Step heading ("Step 6: Code Review") so reviewers can find
and run it.
| "^((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]+)*)$", | ||
| { | ||
| "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 lint error message with allowed prefixes
Line 9 allows tedi-, cdk-, and ng-, but Line 11 says selectors must start with tedi-. Please update the message to reflect the actual accepted patterns; otherwise lint feedback is misleading.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.stylelintrc.json around lines 9 - 11, The lint message for the class
selector rule incorrectly states selectors must start with 'tedi-' even though
the regex pattern "^((tedi|cdk)-...|ng-... )$" allows 'tedi-', 'cdk-' and 'ng-'
prefixes; update the message string (the second item paired with that regex) to
list all allowed prefixes and keep the BEM guidance (e.g., "Class selector must
start with 'tedi-', 'cdk-' or 'ng-' prefix and follow BEM naming (e.g.,
.tedi-button, .cdk-modal, .ng-component, .tedi-button__icon,
.tedi-button--primary). Selector: \"%s\"").
| private openAndFocusFirst() { | ||
| if (!this.dropdown.floatUiComponent().state) { | ||
| if (!this.dropdown.isOpen()) { | ||
| this.dropdown.showDropdown(); | ||
| } | ||
| this.dropdown.focusFirstItem?.(); | ||
| setTimeout(() => this.dropdown.focusFirstItem?.()); | ||
| } | ||
|
|
||
| private openAndFocusLast() { | ||
| if (!this.dropdown.floatUiComponent().state) { | ||
| if (!this.dropdown.isOpen()) { | ||
| this.dropdown.showDropdown(); | ||
| } | ||
| this.dropdown.focusLastItem?.(); | ||
| setTimeout(() => this.dropdown.focusLastItem?.()); |
There was a problem hiding this comment.
Avoid a second deferred focus jump on keyboard open.
DropdownComponent.showDropdown() already queues focusActiveItem() after opening in tedi/components/overlay/dropdown/dropdown.component.ts:74-85. These extra timers add a competing focus change, so ArrowUp/ArrowDown can briefly focus one item and then immediately jump to another. Push the initial-focus choice into the dropdown component (or suppress its default post-open focus for this path) so only one focus pass runs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tedi/components/overlay/dropdown/dropdown-trigger/dropdown-trigger.directive.ts`
around lines 66 - 77, openAndFocusFirst and openAndFocusLast call showDropdown
then schedule an extra setTimeout-based focus (focusFirstItem/focusLastItem)
which conflicts with DropdownComponent.showDropdown's own queued
focusActiveItem; remove the competing timer and instead add an option to
DropdownComponent.showDropdown (e.g., showDropdown({ initialFocus?:
'auto'|'first'|'last' })) or a boolean flag to suppress its default post-open
focusActiveItem so the caller can request first/last focus without a second
deferred focus; update openAndFocusFirst/openAndFocusLast to call showDropdown
with the new option (and remove the setTimeout calls) or, alternatively, expose
a parameter to skip the internal focus and let these methods perform the single
focus pass.
| cdkConnectedOverlay | ||
| [cdkConnectedOverlayOrigin]="overlayOrigin()" | ||
| [cdkConnectedOverlayOpen]="isOpen()" | ||
| [cdkConnectedOverlayPositions]="overlayPositions()" | ||
| (overlayOutsideClick)="onOutsideClick()" |
There was a problem hiding this comment.
Don't steal focus on pointer-driven outside clicks.
Line 7 sends every outside click through onOutsideClick(), and that handler refocuses the trigger (tedi/components/overlay/dropdown/dropdown.component.ts:87-106). Clicking another input or button will therefore close the dropdown and then move focus back to the trigger, which breaks normal pointer interaction outside the menu. Outside clicks should dismiss only; keep trigger refocus for keyboard dismissals such as Escape. The matching focus assertion in the spec will need to be relaxed too.
💡 Minimal fix
- (overlayOutsideClick)="onOutsideClick()"
+ (overlayOutsideClick)="hideDropdown()"📝 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.
| cdkConnectedOverlay | |
| [cdkConnectedOverlayOrigin]="overlayOrigin()" | |
| [cdkConnectedOverlayOpen]="isOpen()" | |
| [cdkConnectedOverlayPositions]="overlayPositions()" | |
| (overlayOutsideClick)="onOutsideClick()" | |
| cdkConnectedOverlay | |
| [cdkConnectedOverlayOrigin]="overlayOrigin()" | |
| [cdkConnectedOverlayOpen]="isOpen()" | |
| [cdkConnectedOverlayPositions]="overlayPositions()" | |
| (overlayOutsideClick)="hideDropdown()" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tedi/components/overlay/dropdown/dropdown.component.html` around lines 3 - 7,
The outside-click handler currently always refocuses the trigger via
onOutsideClick(), breaking pointer interaction; change the template to pass the
event to the handler (overlayOutsideClick)="onOutsideClick($event)" and update
dropdown.component.ts onOutsideClick(event) to only refocus the trigger when the
dismissal is from keyboard (e.g., event is a KeyboardEvent or event.key ===
'Escape') and for pointer-driven events (MouseEvent) simply close the menu
without moving focus; keep existing close logic and ensure any keyboard-driven
escape path still calls the refocus code so keyboard dismissals behave
unchanged.
| afterEach(() => { | ||
| dropdown.hideDropdown(); | ||
| overlayContainerElement.innerHTML = ""; | ||
| }); |
There was a problem hiding this comment.
Restore real timers in teardown.
Several tests later in this file switch to fake timers, for example on Lines 170-177 and Lines 419-480. If any of them fail before jest.useRealTimers(), the suite inherits the fake clock because afterEach() does not reset it. Restoring timers centrally here makes the spec self-healing.
💡 Minimal fix
afterEach(() => {
+ jest.useRealTimers();
dropdown.hideDropdown();
overlayContainerElement.innerHTML = "";
});📝 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.
| afterEach(() => { | |
| dropdown.hideDropdown(); | |
| overlayContainerElement.innerHTML = ""; | |
| }); | |
| afterEach(() => { | |
| jest.useRealTimers(); | |
| dropdown.hideDropdown(); | |
| overlayContainerElement.innerHTML = ""; | |
| }); |
🧰 Tools
🪛 ast-grep (0.42.1)
[warning] 70-70: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: overlayContainerElement.innerHTML = ""
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tedi/components/overlay/dropdown/dropdown.component.spec.ts` around lines 69
- 72, The afterEach teardown currently calls dropdown.hideDropdown() and clears
overlayContainerElement.innerHTML but does not restore Jest timers; add a call
to jest.useRealTimers() in the same afterEach to ensure any tests that switched
to fake timers (e.g., those using fakeAsync/tick) cannot leak a fake clock into
later tests — update the afterEach block that invokes dropdown.hideDropdown()
and overlayContainerElement.innerHTML to also call jest.useRealTimers().
| onOutsideClick() { | ||
| this.hideDropdown(); | ||
| this.dropdownTrigger().host.nativeElement.focus(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the dropdown component file
cat -n tedi/components/overlay/dropdown/dropdown.component.ts | head -150Repository: TEDI-Design-System/angular
Length of output: 4964
🏁 Script executed:
# Let's check the dropdown template to see how onOutsideClick is connected
cat -n tedi/components/overlay/dropdown/dropdown.component.htmlRepository: TEDI-Design-System/angular
Length of output: 624
🏁 Script executed:
# Check the dropdown trigger directive to see keyboard handling
cat -n tedi/components/overlay/dropdown/dropdown-trigger/dropdown-trigger.directive.tsRepository: TEDI-Design-System/angular
Length of output: 2825
Remove unconditional focus restoration from pointer-driven outside clicks.
When overlayOutsideClick fires (user clicks outside the overlay), unconditionally calling focus() on the trigger bounces focus back if the user clicked another button or input. This is a noticeable accessibility regression. Keep focus restoration only for keyboard dismissal (Escape key), which is already handled in the trigger directive.
Suggested fix
onOutsideClick() {
this.hideDropdown();
- this.dropdownTrigger().host.nativeElement.focus();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tedi/components/overlay/dropdown/dropdown.component.ts` around lines 103 -
106, The onOutsideClick handler currently calls this.hideDropdown() and then
unconditionally focuses the trigger element, which reverts focus when the user
clicks another control; remove the call to
this.dropdownTrigger().host.nativeElement.focus() from onOutsideClick so outside
(pointer) clicks simply close the overlay without restoring focus, and leave
keyboard dismissal/focus restoration to the existing Escape/trigger directive
logic (verify the escape handler in the trigger directive retains its focus
behavior). Ensure no other callers rely on onOutsideClick to restore focus and
update any related tests to reflect pointer-driven outside clicks should not
move focus.
| showTooltip() { | ||
| if (!this.floatUiComponent().state) { | ||
| if (!this.isOpen()) { | ||
| clearTimeout(this.hideTimeout); | ||
| this.floatUiComponent().show(); | ||
| this.floatUiDisplay.set("block"); | ||
| this.isOpen.set(true); | ||
| } | ||
| } |
There was a problem hiding this comment.
Always clear the pending hide timer before re-showing.
Line 109 makes clearTimeout(this.hideTimeout) conditional on the tooltip being closed. If pointer or focus re-enters while a hide timeout is already armed, showTooltip() leaves that timer running and the tooltip can still disappear underneath the user even though it is being shown again.
💡 Minimal fix
showTooltip() {
+ clearTimeout(this.hideTimeout);
if (!this.isOpen()) {
- clearTimeout(this.hideTimeout);
this.isOpen.set(true);
}
}📝 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.
| showTooltip() { | |
| if (!this.floatUiComponent().state) { | |
| if (!this.isOpen()) { | |
| clearTimeout(this.hideTimeout); | |
| this.floatUiComponent().show(); | |
| this.floatUiDisplay.set("block"); | |
| this.isOpen.set(true); | |
| } | |
| } | |
| showTooltip() { | |
| clearTimeout(this.hideTimeout); | |
| if (!this.isOpen()) { | |
| this.isOpen.set(true); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tedi/components/overlay/tooltip/tooltip.component.ts` around lines 108 - 113,
The showTooltip() implementation must always cancel any pending hide timer
before attempting to re-open; move or perform clearTimeout(this.hideTimeout)
unconditionally at the start of showTooltip() (before or independent of the
this.isOpen() check) so that an armed hideTimeout cannot remove the tooltip
after re-showing; update the method that references showTooltip,
this.hideTimeout, and this.isOpen to ensure the timer is cleared every time
showTooltip is called.
…loat-ui # Conflicts: # tedi/components/form/filter/filter.component.ts # tedi/components/form/filter/filter.stories.ts # tedi/components/helpers/scroll-fade/scroll-fade.component.scss # tedi/components/helpers/scroll-fade/scroll-fade.component.ts
mart-sessman
temporarily deployed
to
github-pages
GitHub Actions
Inactive
…place-ngx-float-ui
mart-sessman
temporarily deployed
to
github-pages
GitHub Actions
Inactive
1 participant
BREAKING CHANGES: ngx-float-ui dependency removed, appendTo inputs removed from related components
Summary by CodeRabbit
Release Notes
Chores
ngx-float-uidependency from package and documentation.Refactor
appendToinput from overlay components; overlays now append to CDK overlay container by default.Documentation
/simplifycommand for scanning code quality and efficiency.