feat(tabs): add tab-card and tabs-vertical #14#402
Conversation
📝 WalkthroughWalkthroughAdds a vertical tabs feature: a standalone Changes
Sequence DiagramsequenceDiagram
actor User
participant TabCard as TabCardComponent
participant TabsVertical as TabsVerticalComponent
participant TabContent as TabContentComponent
User->>TabCard: click
TabCard->>TabCard: selectTab() sets selected = true
TabCard->>TabsVertical: selection observed via ContentChildren
TabsVertical->>TabsVertical: compute activeTabId / activeTabTitle
TabsVertical->>TabContent: resolve activeTabContent by tabId
TabsVertical->>User: render active content with back button
User->>TabsVertical: click back
TabsVertical->>TabCard: unselectAllTabs() sets selected = false
TabsVertical->>User: render accordion of tab cards
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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.7.0)community/components/navigation/tabs/tab-card/tab-card.component.scssConfigurationError: Could not find "stylelint-config-recess-order". Do you need to install the package or use the "configBasedir" option? community/components/navigation/tabs/tabs-vertical/tabs-vertical.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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
community/components/navigation/tabs/tabs-vertical/tabs-vertical.component.ts (1)
37-47: Consider consolidating the active tab lookups.The
activeTabIdandactiveTabTitlecomputed signals both callthis.tabs().find((tab) => tab.selected())separately. You could deriveactiveTabTitlefrom a single lookup or cache the active tab.♻️ Suggested optimization
+ private activeTab = computed(() => + this.tabs().find((tab) => tab.selected()) + ); + activeTabId = computed(() => - this.tabs().find((tab) => tab.selected())?.tabId() + this.activeTab()?.tabId() ); activeTabTitle = computed(() => - this.tabs().find((tab) => tab.selected())?.title() + this.activeTab()?.title() );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@community/components/navigation/tabs/tabs-vertical/tabs-vertical.component.ts` around lines 37 - 47, Consolidate repeated lookups by creating a single computed signal for the currently selected tab (e.g., activeTab = computed(() => this.tabs().find(tab => tab.selected()))) and then derive activeTabId and activeTabTitle from that computed (use activeTab()?.tabId() and activeTab()?.title()); update activeTabContent to reference activeTab()?.tabId() (or keep using this.activeTabId() if you prefer) so all three computed values reuse the single selection lookup (touch symbols: activeTabId, activeTabTitle, activeTabContent, tabs()).tedi/services/translation/translations.ts (1)
68-73: Consider adding adescriptionfield for consistency.Most translation entries include a
descriptionfield for documentation purposes (e.g., "Used for closing", "For canceling an action"). Adding one here would maintain consistency and help future maintainers understand the translation's purpose."back": { + description: "Used for navigating back in vertical tabs", components: ["Tabs"], et: "Tagasi", en: "Back", ru: "Назад", },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tedi/services/translation/translations.ts` around lines 68 - 73, Add a missing description field to the "back" translation entry in translations.ts: locate the "back" object (components: ["Tabs"], et: "Tagasi", en: "Back", ru: "Назад") and add a descriptive property (e.g., description: "Used to return to the previous screen or navigate back") so it matches the pattern of other translation entries and documents its usage for maintainers.community/components/navigation/tabs/tabs-vertical/tabs-vertical.component.scss (1)
20-22: Consider using more specific selectors.The
> *universal selector applies to all direct children. While functional, you could be more explicit by targeting the specific elements (icon and span) for better maintainability.- &__back-link > * { + &__back-link > tedi-icon, + &__back-link > span { color: var(--button-main-neutral-text-active); }This is a minor suggestion—the current approach works correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@community/components/navigation/tabs/tabs-vertical/tabs-vertical.component.scss` around lines 20 - 22, Replace the broad child selector in the &__back-link block with explicit selectors for the expected child elements to improve maintainability: target the icon element (e.g., svg or .icon) and the text element (e.g., span or .label) instead of using > *; update the selector(s) inside tabs-vertical.component.scss (the &__back-link rule) to list those specific children so only the icon and span inherit color var(--button-main-neutral-text-active).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@community/components/navigation/tabs/tab-card/tab-card.component.html`:
- Around line 4-6: The icon-only button lacks an accessible label; update the
tedi-button in tab-card.component.html (the button containing tedi-icon with
class tedi-tab-card__action-icon) to include an accessible name by adding either
a static aria-label or a bound attribute (e.g. [attr.aria-label]="...") or
include visually hidden text inside the button; if translations are available
bind the aria-label to the translation key so screen readers can announce the
button purpose.
In `@community/components/navigation/tabs/tab-card/tab-card.component.ts`:
- Around line 35-37: The selectTab() method should avoid selecting when the tab
is disabled: add a defensive guard at the start of selectTab() (e.g., if
(this.disabled) return; or if (this.disabled.get && this.disabled.get())
return;) so that the call to this.selected.set(true) is skipped for disabled
tabs; update the selectTab() function to check the tab's disabled state before
setting selected.
- Around line 17-22: The host bindings for the TabCard component are missing an
aria-selected attribute and the BEM modifier class is using the element
convention; update the host metadata to add "[attr.aria-selected]":
"isSelected()" (or an existing selection boolean) alongside the existing
"[class.tedi-tab-card__disabled]": "disabledInput()" and ensure the disabled
class uses the modifier name "[class.tedi-tab-card--disabled]":
"disabledInput()"; also update the SCSS selector from &__disabled to &--disabled
so styles match the new class; keep existing (click) handler selectTab()
unchanged.
---
Nitpick comments:
In
`@community/components/navigation/tabs/tabs-vertical/tabs-vertical.component.scss`:
- Around line 20-22: Replace the broad child selector in the &__back-link block
with explicit selectors for the expected child elements to improve
maintainability: target the icon element (e.g., svg or .icon) and the text
element (e.g., span or .label) instead of using > *; update the selector(s)
inside tabs-vertical.component.scss (the &__back-link rule) to list those
specific children so only the icon and span inherit color
var(--button-main-neutral-text-active).
In
`@community/components/navigation/tabs/tabs-vertical/tabs-vertical.component.ts`:
- Around line 37-47: Consolidate repeated lookups by creating a single computed
signal for the currently selected tab (e.g., activeTab = computed(() =>
this.tabs().find(tab => tab.selected()))) and then derive activeTabId and
activeTabTitle from that computed (use activeTab()?.tabId() and
activeTab()?.title()); update activeTabContent to reference activeTab()?.tabId()
(or keep using this.activeTabId() if you prefer) so all three computed values
reuse the single selection lookup (touch symbols: activeTabId, activeTabTitle,
activeTabContent, tabs()).
In `@tedi/services/translation/translations.ts`:
- Around line 68-73: Add a missing description field to the "back" translation
entry in translations.ts: locate the "back" object (components: ["Tabs"], et:
"Tagasi", en: "Back", ru: "Назад") and add a descriptive property (e.g.,
description: "Used to return to the previous screen or navigate back") so it
matches the pattern of other translation entries and documents its usage for
maintainers.
🪄 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: c69ecc56-f18c-401c-a148-d000dd9b1e67
📒 Files selected for processing (8)
community/components/navigation/tabs/tab-card/tab-card.component.htmlcommunity/components/navigation/tabs/tab-card/tab-card.component.scsscommunity/components/navigation/tabs/tab-card/tab-card.component.tscommunity/components/navigation/tabs/tabs-vertical/tabs-vertical.component.htmlcommunity/components/navigation/tabs/tabs-vertical/tabs-vertical.component.scsscommunity/components/navigation/tabs/tabs-vertical/tabs-vertical.component.tscommunity/components/navigation/tabs/tabs.stories.tstedi/services/translation/translations.ts
svekko
force-pushed
the
feat_tab_card
branch
3 times, most recently
from
acd1241 to
3dac192
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
community/components/navigation/tabs/tabs-vertical/tabs-vertical.component.ts (1)
37-43: Derive active tab once to avoid duplicate scans.Line 37 and Line 41 repeat the same
findovertabs(). Consider a single computedselectedTaband derive both id/title from it for cleaner, consistent state derivation.♻️ Proposed refactor
export class TabsVerticalComponent { private readonly tabs = contentChildren(TabCardComponent); private readonly tabContents = contentChildren(TabContentComponent); - activeTabId = computed(() => - this.tabs().find((tab) => tab.selected())?.tabId() - ); + private readonly selectedTab = computed(() => + this.tabs().find((tab) => tab.selected()) + ); - activeTabTitle = computed(() => - this.tabs().find((tab) => tab.selected())?.title() - ); + activeTabId = computed(() => this.selectedTab()?.tabId()); + + activeTabTitle = computed(() => this.selectedTab()?.title()); activeTabContent = computed(() => this.tabContents().find((content) => content.tabId() === this.activeTabId())?.content() );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@community/components/navigation/tabs/tabs-vertical/tabs-vertical.component.ts` around lines 37 - 43, Replace the duplicated find over tabs() by adding a single computed (e.g., selectedTab) that returns this.tabs().find(tab => tab.selected()), then derive activeTabId and activeTabTitle from that computed (use selectedTab()?.tabId() and selectedTab()?.title()); update references of activeTabId and activeTabTitle to use the new selectedTab computed to avoid scanning tabs() twice and keep derived state consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@community/components/navigation/tabs/tabs-vertical/tabs-vertical.component.html`:
- Line 6: The back-link button in the tabs-vertical template is missing an
explicit type and may default to "submit" within forms; update the button
element (the one with class "tedi-tabs-vertical__back-link" and click handler
unselectAllTabs()) to include type="button" so it won't submit forms
inadvertently.
---
Nitpick comments:
In
`@community/components/navigation/tabs/tabs-vertical/tabs-vertical.component.ts`:
- Around line 37-43: Replace the duplicated find over tabs() by adding a single
computed (e.g., selectedTab) that returns this.tabs().find(tab =>
tab.selected()), then derive activeTabId and activeTabTitle from that computed
(use selectedTab()?.tabId() and selectedTab()?.title()); update references of
activeTabId and activeTabTitle to use the new selectedTab computed to avoid
scanning tabs() twice and keep derived state consistent.
🪄 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: 80e0b64f-ad00-4996-b057-f6ec6e34fae4
📒 Files selected for processing (8)
community/components/navigation/tabs/tab-card/tab-card.component.htmlcommunity/components/navigation/tabs/tab-card/tab-card.component.scsscommunity/components/navigation/tabs/tab-card/tab-card.component.tscommunity/components/navigation/tabs/tabs-vertical/tabs-vertical.component.htmlcommunity/components/navigation/tabs/tabs-vertical/tabs-vertical.component.scsscommunity/components/navigation/tabs/tabs-vertical/tabs-vertical.component.tscommunity/components/navigation/tabs/tabs.stories.tstedi/services/translation/translations.ts
✅ Files skipped from review due to trivial changes (2)
- community/components/navigation/tabs/tabs-vertical/tabs-vertical.component.scss
- community/components/navigation/tabs/tab-card/tab-card.component.scss
🚧 Files skipped from review as they are similar to previous changes (3)
- tedi/services/translation/translations.ts
- community/components/navigation/tabs/tabs.stories.ts
- community/components/navigation/tabs/tab-card/tab-card.component.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@community/components/navigation/tabs/tab-card/tab-card.component.ts`:
- Around line 29-43: The selectTab() method on TabCardComponent currently only
sets this.selected to true, allowing multiple tabs to be selected; update
TabCardComponent to emit an output event when a tab is clicked (e.g., add an
EventEmitter like select = new EventEmitter<TabCardComponent>() and fire it from
selectTab() after the disabledInput() check) and have the parent
TabsVerticalComponent listen for that event and call its existing
unselectAllTabs() then set the clicked tab's selected model to true;
alternatively implement an effect in TabCardComponent that notifies the parent
(via the new output) so the parent can enforce mutual exclusion using
TabsVerticalComponent.unselectAllTabs() and then mark the emitting
TabCardComponent.selected.set(true).
🪄 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: 87b91fe6-2f74-4463-bda2-9f48c4c5edf5
📒 Files selected for processing (8)
community/components/navigation/tabs/tab-card/tab-card.component.htmlcommunity/components/navigation/tabs/tab-card/tab-card.component.scsscommunity/components/navigation/tabs/tab-card/tab-card.component.tscommunity/components/navigation/tabs/tabs-vertical/tabs-vertical.component.htmlcommunity/components/navigation/tabs/tabs-vertical/tabs-vertical.component.scsscommunity/components/navigation/tabs/tabs-vertical/tabs-vertical.component.tscommunity/components/navigation/tabs/tabs.stories.tstedi/services/translation/translations.ts
✅ Files skipped from review due to trivial changes (3)
- community/components/navigation/tabs/tab-card/tab-card.component.scss
- community/components/navigation/tabs/tabs-vertical/tabs-vertical.component.scss
- tedi/services/translation/translations.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- community/components/navigation/tabs/tabs-vertical/tabs-vertical.component.ts
- community/components/navigation/tabs/tabs.stories.ts
| selector: '[tedi-tab-card]', | ||
| imports: [ | ||
| ButtonComponent, | ||
| CardComponent, | ||
| CardContentComponent, | ||
| IconComponent | ||
| ], | ||
| templateUrl: './tab-card.component.html', | ||
| styleUrl: './tab-card.component.scss', | ||
| encapsulation: ViewEncapsulation.None, | ||
| changeDetection: ChangeDetectionStrategy.OnPush, | ||
| host: { | ||
| "[class.tedi-tab-card]": "true", | ||
| "[class.tedi-tab-card--disabled]": "disabledInput()", | ||
| "[attr.role]": "'tab'", | ||
| "[attr.aria-selected]": "selected()", | ||
| "[attr.aria-disabled]": "disabledInput()", | ||
| "(click)": "selectTab()", | ||
| }, |
There was a problem hiding this comment.
Keyboard activation and tab-pattern semantics are incomplete.
With selector: '[tedi-tab-card]' and role="tab" on the host, activation relies on whatever element the attribute is applied to. If the selector is used on a non-focusable element (e.g., <div tedi-tab-card>), users cannot select tabs via Enter/Space or arrow keys, and the element won't be in the tab order. Additionally, per the ARIA tab pattern the host should expose aria-controls pointing to the active tabpanel and manage roving tabindex.
Suggestions:
- Add
(keydown.enter)/(keydown.space)host listeners that callselectTab()(with$event.preventDefault()for Space). - Bind
[attr.tabindex](0 for selected/default, -1 otherwise) so arrow-key navigation can be layered on later. - Consider binding
[attr.aria-controls]to the id of the rendered tabpanel inTabsVerticalComponent.
| readonly selected = model(false); | ||
|
|
||
| readonly disabledInput = input(false, { | ||
| transform: booleanAttribute, | ||
| // eslint-disable-next-line @angular-eslint/no-input-rename | ||
| alias: "disabled", | ||
| }); | ||
|
|
||
| selectTab() { | ||
| if (this.disabledInput()) { | ||
| return; | ||
| } | ||
|
|
||
| this.selected.set(true); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -e ts 'tabs-vertical\.component\.ts$' | xargs rg -nC3 "selected|activeTab|contentChildren"Repository: TEDI-Design-System/angular
Length of output: 1028
🏁 Script executed:
#!/bin/bash
# Find the complete TabsVerticalComponent to see if there are effects or outputs managing selection
fd -e ts 'tabs-vertical\.component\.ts$' -exec cat -n {} \;Repository: TEDI-Design-System/angular
Length of output: 300
🏁 Script executed:
#!/bin/bash
# Search for calls to unselectAllTabs or any selection-related logic
rg -r 'unselectAllTabs|selectTab' --type ts -B2 -A2Repository: TEDI-Design-System/angular
Length of output: 466
🏁 Script executed:
#!/bin/bash
# Check if there are any stories or examples showing the tab usage pattern
fd -e ts -e html 'tab.*story\|tab.*spec' | head -20Repository: TEDI-Design-System/angular
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Find and display the complete TabsVerticalComponent
fd -e ts 'tabs-vertical\.component\.ts$' -exec cat {} \;Repository: TEDI-Design-System/angular
Length of output: 257
🏁 Script executed:
#!/bin/bash
# Search for calls to unselectAllTabs or selectTab with context
rg 'unselectAllTabs|selectTab' -B2 -A2 --type tsRepository: TEDI-Design-System/angular
Length of output: 2297
🏁 Script executed:
#!/bin/bash
# Find tab-related stories and specs
fd '(tab|tabs).*\.(spec|stories)\.ts$'Repository: TEDI-Design-System/angular
Length of output: 252
🏁 Script executed:
#!/bin/bash
# Read the complete TabsVerticalComponent file
cat community/components/navigation/tabs/tabs-vertical/tabs-vertical.component.tsRepository: TEDI-Design-System/angular
Length of output: 1620
🏁 Script executed:
#!/bin/bash
# Read the tab-card component to see the full implementation
cat community/components/navigation/tabs/tab-card/tab-card.component.tsRepository: TEDI-Design-System/angular
Length of output: 1374
🏁 Script executed:
#!/bin/bash
# Check the stories file for usage patterns
cat community/components/navigation/tabs/tabs.stories.tsRepository: TEDI-Design-System/angular
Length of output: 4392
Multiple tabs can be selected = true simultaneously, breaking the tab pattern.
selectTab() only sets selected to true and never deselects siblings. Clicking tab A then tab B leaves both with selected=true (and aria-selected="true"). While TabsVerticalComponent has an unselectAllTabs() method, it is never called—there's no output event from TabCardComponent to trigger parent deselection, and no effect managing mutual exclusion.
Add an output event from TabCardComponent.selectTab() and have the parent respond by calling unselectAllTabs() and setting the clicked tab as selected, or use an effect to automatically manage deselection when a tab is selected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@community/components/navigation/tabs/tab-card/tab-card.component.ts` around
lines 29 - 43, The selectTab() method on TabCardComponent currently only sets
this.selected to true, allowing multiple tabs to be selected; update
TabCardComponent to emit an output event when a tab is clicked (e.g., add an
EventEmitter like select = new EventEmitter<TabCardComponent>() and fire it from
selectTab() after the disabledInput() check) and have the parent
TabsVerticalComponent listen for that event and call its existing
unselectAllTabs() then set the clicked tab's selected model to true;
alternatively implement an effect in TabCardComponent that notifies the parent
(via the new output) so the parent can enforce mutual exclusion using
TabsVerticalComponent.unselectAllTabs() and then mark the emitting
TabCardComponent.selected.set(true).
2 participants
feat(tabs): add tab-card and tabs-vertical #14: Add tab-card and tabs-vertical component
Summary by CodeRabbit
New Features
Style