feat(table-of-contents): add new TEDI-Ready component #543 - #551
feat(table-of-contents): add new TEDI-Ready component #543#551ly-tempel-bitweb wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a standalone desktop and collapsible Table of Contents implementation with nested items, active-branch expansion, numbering, validation icons, accessibility behavior, styling, tests, Storybook examples, exports, translations, documentation, and deprecation metadata for community equivalents. ChangesTable of Contents implementation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant HostComponent
participant TableOfContentsCollapsibleComponent
participant Overlay
participant Dialog
HostComponent->>TableOfContentsCollapsibleComponent: Click bottom-bar toggle
TableOfContentsCollapsibleComponent->>Overlay: Configure bottom position
TableOfContentsCollapsibleComponent->>Dialog: Open sheet with projected items
Dialog-->>TableOfContentsCollapsibleComponent: Emit close event
TableOfContentsCollapsibleComponent->>TableOfContentsCollapsibleComponent: Clear open dialog reference
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
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: 4
🧹 Nitpick comments (4)
tedi/components/navigation/table-of-contents/table-of-contents.component.ts (1)
94-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winApply the required component member ordering consistently.
tedi/components/navigation/table-of-contents/table-of-contents.component.ts#L94-L94: movetranslationsbefore the public inputs.tedi/components/navigation/table-of-contents/table-of-contents-item/table-of-contents-item.component.ts#L56-L61: move all injected dependencies before the public inputs.As per path instructions, injected services must precede inputs/signals.
🤖 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/navigation/table-of-contents/table-of-contents.component.ts` at line 94, Reorder component members so injected dependencies precede public inputs and signals: move the translations injection in tedi/components/navigation/table-of-contents/table-of-contents.component.ts (lines 94-94) before the public inputs, and move all injected dependencies in tedi/components/navigation/table-of-contents/table-of-contents-item/table-of-contents-item.component.ts (lines 56-61) before its public inputs, without changing behavior.Source: Path instructions
tedi/components/navigation/table-of-contents/table-of-contents-collapsible/table-of-contents-collapsible.component.ts (2)
78-80: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove injected services to the top of the class.
As per path instructions, component architecture must follow the order:
injected services → inputs/signals → outputs → internal signals/computed → lifecycle → public/private methods. Please move these injected services above theinput()declarations.🤖 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/navigation/table-of-contents/table-of-contents-collapsible/table-of-contents-collapsible.component.ts` around lines 78 - 80, Reorder the class members in the table-of-contents collapsible component so the injected services (`dialog`, `overlay`, and `translations`) appear before all `input()` declarations, followed by the remaining members in the prescribed architecture order.Source: Path instructions
93-97: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify redundant nullish coalescing logic.
The
resolved ?? this.titleLabel()operation treats an explicitnullas a fallback trigger. Since bothundefinedandnullwill fall back tothis.titleLabel(), the explicitundefinedcheck is redundant and can be simplified.♻️ Proposed refactor
readonly title = computed(() => { - const heading = this.heading(); - const resolved = heading === undefined ? this.titleLabel() : heading; - return resolved ?? this.titleLabel(); + return this.heading() ?? this.titleLabel(); });🤖 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/navigation/table-of-contents/table-of-contents-collapsible/table-of-contents-collapsible.component.ts` around lines 93 - 97, The title computed value redundantly checks for undefined and then applies nullish fallback. Simplify the logic in the title computed property so both null and undefined heading values fall back directly to this.titleLabel(), while preserving non-null heading values unchanged.tedi/components/navigation/table-of-contents/table-of-contents-collapsible/table-of-contents-collapsible.component.spec.ts (1)
36-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffMock
TediTranslationServiceand the CDK dialog/overlay. The spec should use a local translation stub instead of the real service, and a dialog/overlay test double instead of opening the sheet intodocument.body.🤖 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/navigation/table-of-contents/table-of-contents-collapsible/table-of-contents-collapsible.component.spec.ts` around lines 36 - 40, Update the table-of-contents-collapsible test setup in beforeEach to provide a local TediTranslationService mock and CDK dialog/overlay test doubles. Replace the real service and document.body-backed sheet behavior while preserving the existing HostComponent test flow and translation token configuration.Source: Path instructions
🤖 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/navigation/table-of-contents/table-of-contents-collapsible/table-of-contents-collapsible.component.ts`:
- Line 51: Update TableOfContentsCollapsibleComponent to implement Angular’s
OnDestroy lifecycle interface, import it from `@angular/core`, and add
ngOnDestroy() that calls the existing close() method so any open dialog is
closed when the component is destroyed.
In
`@tedi/components/navigation/table-of-contents/table-of-contents.component.scss`:
- Around line 104-113: Update the selected-state rule under
.tedi-table-of-contents__item--selected > .tedi-table-of-contents__row to
include the established projected-icon inherited-color selector, ensuring
projected icons within selected links or buttons inherit
var(--link-primary-active) alongside the text color.
In
`@tedi/components/navigation/table-of-contents/table-of-contents.component.spec.ts`:
- Around line 75-79: Update the test setup in setup to provide a deterministic
mock TediTranslationService with track() and translate() methods, alongside the
existing TEDI_TRANSLATION_DEFAULT_TOKEN provider. Ensure translated component
tests use the mock service instead of the real implementation.
In `@tedi/components/navigation/table-of-contents/table-of-contents.stories.ts`:
- Around line 213-242: Update the scroll-spy implementation around the
IntersectionObserver and scroll handler at
tedi/components/navigation/table-of-contents/table-of-contents.stories.ts:213-242
to retain the listener reference and release all resources during destruction:
disconnect the observer, remove the stored scroll listener, and clear
seekEndTimeout. Apply the corresponding observer-destruction cleanup to the
copy-pasteable source example at
tedi/components/navigation/table-of-contents/table-of-contents.stories.ts:823-837.
---
Nitpick comments:
In
`@tedi/components/navigation/table-of-contents/table-of-contents-collapsible/table-of-contents-collapsible.component.spec.ts`:
- Around line 36-40: Update the table-of-contents-collapsible test setup in
beforeEach to provide a local TediTranslationService mock and CDK dialog/overlay
test doubles. Replace the real service and document.body-backed sheet behavior
while preserving the existing HostComponent test flow and translation token
configuration.
In
`@tedi/components/navigation/table-of-contents/table-of-contents-collapsible/table-of-contents-collapsible.component.ts`:
- Around line 78-80: Reorder the class members in the table-of-contents
collapsible component so the injected services (`dialog`, `overlay`, and
`translations`) appear before all `input()` declarations, followed by the
remaining members in the prescribed architecture order.
- Around line 93-97: The title computed value redundantly checks for undefined
and then applies nullish fallback. Simplify the logic in the title computed
property so both null and undefined heading values fall back directly to
this.titleLabel(), while preserving non-null heading values unchanged.
In `@tedi/components/navigation/table-of-contents/table-of-contents.component.ts`:
- Line 94: Reorder component members so injected dependencies precede public
inputs and signals: move the translations injection in
tedi/components/navigation/table-of-contents/table-of-contents.component.ts
(lines 94-94) before the public inputs, and move all injected dependencies in
tedi/components/navigation/table-of-contents/table-of-contents-item/table-of-contents-item.component.ts
(lines 56-61) before its public inputs, without changing behavior.
🪄 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: d3ada7f0-21e3-457d-8d60-66df663bcee6
📒 Files selected for processing (18)
community/components/navigation/table-of-contents/table-of-contents-item/table-of-contents-item.component.tscommunity/components/navigation/table-of-contents/table-of-contents-nested-wrapper.component.tscommunity/components/navigation/table-of-contents/table-of-contents.component.tscommunity/components/navigation/table-of-contents/table-of-contents.stories.tsskills/tedi-angular/references/components.mdtedi/components/navigation/index.tstedi/components/navigation/table-of-contents/index.tstedi/components/navigation/table-of-contents/table-of-contents-collapsible/table-of-contents-collapsible.component.htmltedi/components/navigation/table-of-contents/table-of-contents-collapsible/table-of-contents-collapsible.component.spec.tstedi/components/navigation/table-of-contents/table-of-contents-collapsible/table-of-contents-collapsible.component.tstedi/components/navigation/table-of-contents/table-of-contents-item/table-of-contents-item.component.htmltedi/components/navigation/table-of-contents/table-of-contents-item/table-of-contents-item.component.tstedi/components/navigation/table-of-contents/table-of-contents.component.htmltedi/components/navigation/table-of-contents/table-of-contents.component.scsstedi/components/navigation/table-of-contents/table-of-contents.component.spec.tstedi/components/navigation/table-of-contents/table-of-contents.component.tstedi/components/navigation/table-of-contents/table-of-contents.stories.tstedi/services/translation/translations.ts
| .tedi-table-of-contents__item--selected > .tedi-table-of-contents__row { | ||
| border-left-color: var(--general-border-brand); | ||
|
|
||
| .tedi-table-of-contents__number { | ||
| color: var(--link-primary-active); | ||
| } | ||
|
|
||
| .tedi-table-of-contents__content :is(a, button) { | ||
| color: var(--link-primary-active); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep projected icons synchronized with the selected text color.
The selected state changes the link/button color but provides no matching override for projected icons, so a link icon can retain its default color. Add the established inherited-color rule for projected icons in this state.
As per coding guidelines, projected icons must inherit parent hover or selected/active colors.
🤖 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/navigation/table-of-contents/table-of-contents.component.scss`
around lines 104 - 113, Update the selected-state rule under
.tedi-table-of-contents__item--selected > .tedi-table-of-contents__row to
include the established projected-icon inherited-color selector, ensuring
projected icons within selected links or buttons inherit
var(--link-primary-active) alongside the text color.
Sources: Coding guidelines, Path instructions
| const setup = async (component: unknown) => { | ||
| await TestBed.configureTestingModule({ | ||
| imports: [component as never], | ||
| providers: [{ provide: TEDI_TRANSLATION_DEFAULT_TOKEN, useValue: "en" }], | ||
| }).compileComponents(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Mock TediTranslationService in translated component tests.
Provide a deterministic mock for the track() and translate() calls alongside TEDI_TRANSLATION_DEFAULT_TOKEN, rather than exercising the real service.
As per coding guidelines, translated component tests must mock TediTranslationService and provide TEDI_TRANSLATION_DEFAULT_TOKEN.
🤖 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/navigation/table-of-contents/table-of-contents.component.spec.ts`
around lines 75 - 79, Update the test setup in setup to provide a deterministic
mock TediTranslationService with track() and translate() methods, alongside the
existing TEDI_TRANSLATION_DEFAULT_TOKEN provider. Ensure translated component
tests use the mock service instead of the real implementation.
Sources: Coding guidelines, Path instructions
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
GitHub Actions
Inactive
1 participant
Summary by CodeRabbit
New Features
Documentation
Deprecation