feat(header): update Header component against Figma #312#440
Open
ly-tempel-bitweb wants to merge 4 commits into
Open
feat(header): update Header component against Figma #312#440ly-tempel-bitweb wants to merge 4 commits into
ly-tempel-bitweb wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughHeader subsystem: adds Storybook theme provider; introduces HeaderMobileButton, HeaderLogo (+dark directive), HeaderBottom, HeaderSearch; refactors HeaderLogin/HeaderLogout/Profile/Role to breakpoint-aware, input-driven APIs; updates header templates, styles, popover reactive state, translations, and Storybook stories/tests. ChangesSupport Infrastructure & Foundation
New Header Foundation Components
Header Action Buttons Refactoring
Header Profile & Role Components
Header Layout & Styling System
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
ly-tempel-bitweb
temporarily deployed
to
github-pages
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tedi/components/layout/header/header-profile/header-profile.component.spec.ts (1)
15-22:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing
TediTranslationServicemock in TestBed providers.The coding guidelines require providing both
TediTranslationServicemock andTEDI_TRANSLATION_DEFAULT_TOKENin test setup for components using translations. Currently, only the token is provided (line 19), but the service mock is missing from the providers array. The test on lines 75-77 spies oncomponent.translationService, indicating the service is expected to be available. Providing a mock upfront ensures consistent, isolated test behavior rather than relying on per-test spies or real service instances.As per coding guidelines,
tedi/**/*.component.spec.ts: ProvideTediTranslationServicemock andTEDI_TRANSLATION_DEFAULT_TOKENin test setup for tests using translated components.🧪 Proposed fix to add TediTranslationService mock
+import { TediTranslationService } from "../../../../services/translation/translation.service"; + describe("HeaderProfileComponent", () => { let fixture: ComponentFixture<HeaderProfileComponent>; let component: HeaderProfileComponent; let documentMock: Document; + let translationServiceMock: jest.Mocked<TediTranslationService>; beforeEach(() => { documentMock = document; + translationServiceMock = { + translate: jest.fn((key: string) => `__${key}__`), + } as unknown as jest.Mocked<TediTranslationService>; TestBed.configureTestingModule({ imports: [HeaderProfileComponent], providers: [ { provide: DOCUMENT, useValue: documentMock }, { provide: TEDI_TRANSLATION_DEFAULT_TOKEN, useValue: "et" }, + { provide: TediTranslationService, useValue: translationServiceMock }, ], schemas: [NO_ERRORS_SCHEMA], });Then simplify the spy in the
resolvedLabeltest:it("falls back to the `header.profile` translation key when `label` is empty", () => { - const translate = jest - .spyOn(component.translationService, "translate") - .mockImplementation(((...args: unknown[]) => `__${args[0]}__`) as unknown as typeof component.translationService.translate); - fixture.componentRef.setInput("label", ""); fixture.detectChanges(); expect(component.resolvedLabel()).toBe("__header.profile__"); - expect(translate).toHaveBeenCalledWith("header.profile"); + expect(translationServiceMock.translate).toHaveBeenCalledWith("header.profile"); });🤖 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/layout/header/header-profile/header-profile.component.spec.ts` around lines 15 - 22, Add a mock provider for TediTranslationService to the TestBed.configureTestingModule providers list alongside the existing TEDI_TRANSLATION_DEFAULT_TOKEN so the component has a predictable translation service instance; update the providers array to include a mock object (or jasmine.createSpyObj) for TediTranslationService and remove or simplify per-test spies that access component.translationService (e.g., the spy in the resolvedLabel test) so tests use the injected mock consistently.tedi/components/layout/header/header-language/header-language.component.spec.ts (1)
25-31: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winInclude
TEDI_TRANSLATION_DEFAULT_TOKENin TestBed providers.This spec exercises a translated component; add the default translation token alongside the service mock to match required test setup.
Proposed patch
import { HeaderLanguageComponent, HeaderLanguage } from './header-language.component'; import { Language, TediTranslationService, + TEDI_TRANSLATION_DEFAULT_TOKEN, } from '../../../../services/translation/translation.service'; @@ providers: [ - { provide: TediTranslationService, useValue: mockTranslationService } + { provide: TediTranslationService, useValue: mockTranslationService }, + { provide: TEDI_TRANSLATION_DEFAULT_TOKEN, useValue: 'et' }, ],As per coding guidelines, “Provide
TediTranslationServicemock andTEDI_TRANSLATION_DEFAULT_TOKENin test setup for tests using translated components.”🤖 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/layout/header/header-language/header-language.component.spec.ts` around lines 25 - 31, The TestBed setup for HeaderLanguageComponent is missing the TEDI_TRANSLATION_DEFAULT_TOKEN provider; update the providers array in the TestBed.configureTestingModule call to include { provide: TEDI_TRANSLATION_DEFAULT_TOKEN, useValue: /* default token value or mock */ } alongside the existing { provide: TediTranslationService, useValue: mockTranslationService } so the translated component has the required default token during tests.
🧹 Nitpick comments (5)
tedi/components/layout/header/header-role/header-role.component.ts (1)
140-144: ⚖️ Poor tradeoffConsider using
afterNextRenderfor focus timing.The
setTimeoutwithout a delay relies on the next event loop tick to ensure the search input is rendered. While this typically works, Angular'safterNextRenderAPI provides more explicit timing control for post-render operations.♻️ Alternative implementation
-import { _IdGenerator } from "@angular/cdk/a11y"; +import { _IdGenerator } from "@angular/cdk/a11y"; +import { afterNextRender } from "@angular/core";- effect(() => { - if (this.popover()?.isOpen() && this.showInput()) { - setTimeout(() => this.searchInput()?.nativeElement.focus()); - } - }); + effect(() => { + if (this.popover()?.isOpen() && this.showInput()) { + afterNextRender(() => { + this.searchInput()?.nativeElement.focus(); + }); + } + });🤖 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/layout/header/header-role/header-role.component.ts` around lines 140 - 144, The effect callback currently uses setTimeout(() => this.searchInput()?.nativeElement.focus()) to wait for the input to be rendered; replace that timing hack with Angular's afterNextRender to run the focus after the next render cycle. Import afterNextRender from `@angular/core` and inside effect() where you check this.popover()?.isOpen() && this.showInput(), call afterNextRender(() => this.searchInput()?.nativeElement.focus()) instead of setTimeout; keep the same null-safe calls to popover, showInput and searchInput so behavior is preserved.tedi/components/layout/header/header-profile/header-profile.component.ts (1)
110-116: 💤 Low valueVerify
resolvedLabelusage aligns with component behavior.The
resolvedLabelcomputed property returnstranslationService.translate("header.profile")whenlabel()is empty, but the desktop button template (in the HTML file) only displays the label whenlabel()is truthy. This meansresolvedLabel()is only used by the mobile button, which is correct. However, the naming might be misleading since "resolved" implies it's used everywhere.Consider renaming to
mobileLabelor adding a JSDoc comment clarifying it's primarily for the mobile button variant.🤖 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/layout/header/header-profile/header-profile.component.ts` around lines 110 - 116, resolvedLabel returns a fallback translation for mobile use but the desktop template ignores it, so rename the computed property or document its intent: update the computed property name from resolvedLabel to mobileLabel (and update all template and TS references) OR add a brief JSDoc above resolvedLabel clarifying “used only by the mobile button; desktop uses label() directly,” and ensure translationService.translate("header.profile") remains the fallback; reference the computed symbol resolvedLabel/mobileLabel, the label() accessor, and translationService.translate when making the change.tedi/components/layout/header/header.component.scss (1)
3-3: ⚡ Quick winRemove fallback value from CSS variable usage
var(--tedi-alpha-20, rgb(0 0 0 / 20%))violates the SCSS rule disallowing fallback values invar().Proposed fix
- box-shadow: 0 1px 5px 0 var(--tedi-alpha-20, rgb(0 0 0 / 20%)); + box-shadow: 0 1px 5px 0 var(--tedi-alpha-20);As per coding guidelines: “Do not use fallback values in CSS
var()functions — writevar(--token-name)without fallback values”.🤖 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/layout/header/header.component.scss` at line 3, The CSS uses a fallback in the var() call for the box-shadow: replace the fallback usage var(--tedi-alpha-20, rgb(0 0 0 / 20%)) with a bare variable reference var(--tedi-alpha-20) so the SCSS rule is satisfied; update the box-shadow declaration that references --tedi-alpha-20 to remove the fallback and ensure the design token is defined elsewhere.tedi/components/layout/header/header-logout/header-logout.component.ts (1)
37-37: 💤 Low valueConsider making
breakpointServiceprivate for consistency.Line 36 marks
translationServiceasprivate, andbreakpointServiceis only used internally. For consistency and encapsulation, consider marking itprivate readonly.♻️ Proposed refactor
- breakpointService = inject(BreakpointService); + private readonly breakpointService = inject(BreakpointService);🤖 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/layout/header/header-logout/header-logout.component.ts` at line 37, The BreakpointService instance assigned to breakpointService should be made private and readonly for consistency and encapsulation: change the injected property breakpointService (type BreakpointService) to private readonly, similar to translationService, since breakpointService is only used internally within HeaderLogoutComponent; update any references accordingly if needed.tedi/components/layout/header/header-login/header-login.component.spec.ts (1)
68-68: ⚡ Quick winUse
setInputfor consistency with other tests.For consistency with the rest of the test suite (lines 95, 124), use
fixture.componentRef.setInput('label', 'Sign in')instead of direct property assignment.♻️ Proposed refactor
- fixture.componentInstance.label = "Sign in"; + fixture.componentRef.setInput("label", "Sign in");🤖 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/layout/header/header-login/header-login.component.spec.ts` at line 68, Replace the direct property assignment to the test component instance (fixture.componentInstance.label = "Sign in") with the consistent test API by calling fixture.componentRef.setInput('label', 'Sign in'); locate the occurrence in header-login.component.spec.ts where fixture.componentInstance is used and change it to use fixture.componentRef.setInput so it matches other tests that set inputs via setInput.
🤖 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/layout/header/header-language/header-language.component.scss`:
- Around line 10-17: The SCSS is styling the Angular element selector
tedi-icon[name="expand_more"] directly; instead add a host CSS class (e.g.,
tedi-header-language__chevron) to the icon in the header-language template and
update the SCSS selectors to target that class under [tedi-popover-trigger] and
[tedi-popover-trigger][aria-expanded="true"] (replace
tedi-icon[name="expand_more"] with .tedi-header-language__chevron in both
rules). Ensure the template adds the class to the icon host and the SCSS uses
the new class for margin, transition and transform.
In `@tedi/components/layout/header/header-login/header-login.component.html`:
- Line 17: In header-login.component.html, update the button element used for
the header login (the <button> with tedi-button and class
"tedi-header-login__button") to include an explicit type="button" attribute so
it does not trigger form submission when placed inside a form; locate the button
in the header-login template and add the type attribute to the element that uses
the tedi-button directive/class.
In `@tedi/components/layout/header/header-profile/header-profile.component.ts`:
- Around line 54-92: Update the public changelog and migration guide to call out
the input API changes for HeaderProfileComponent: state that the inputs name and
showDropdown were removed and list the new inputs label, showPopover, noStyle,
and size (reference the component inputs label, showPopover, noStyle, size in
header-profile.component.ts); provide a short migration snippet and recommended
replacements (e.g., replace name with label or elsewhere, replace showDropdown
logic with showPopover/modal behavior) and mark this as a breaking change in the
release notes with the component name and version bump.
In `@tedi/components/layout/header/header-role/header-role.component.ts`:
- Around line 39-55: Representative.id was made required which breaks callers;
update documentation and add a runtime compatibility fix: document the breaking
change in release notes/migration guide and, in the component that consumes the
type (e.g., HeaderRoleComponent handling the representatives input and selection
logic such as the code paths that sync selection or call
setSelectedRepresentative), detect missing id values and synthesize a stable id
(for example from name or index or a UUID) before using them for
comparison/selection, or revert the type to keep id optional until consumers are
migrated. Ensure references to the Representative type and its id field are
updated accordingly.
In `@tedi/components/layout/header/header-search/header-search.component.ts`:
- Around line 103-107: The effect that currently closes the modal when not
mobile leaves modalOpen true when the variant flips away from "modal", causing a
stale reopen later; update the effect (the reactive effect using this.isMobile()
and this.modalOpen()) to also reset this.modalOpen to false whenever the
mobileVariant is not "modal" (e.g., check this.mobileVariant() !== "modal" or
add a helper like isModalVariant()), ensuring modalOpen is explicitly set false
when variant changes away from modal so it cannot reopen unexpectedly; reference
the existing effect, isMobile(), modalOpen, and mobileVariant symbols when
making the change.
In `@tedi/components/layout/header/header.stories.ts`:
- Around line 1004-1005: In the WithSearch2 story update the two tedi-search
instances so they do not share the same inputId ("search-4"); give each a unique
id (e.g., change the second tedi-search inputId to "search-5") to avoid
duplicate-id accessibility and interaction conflicts—update the inputId
attributes on the tedi-search elements referenced in the WithSearch2 story (also
update the corresponding second occurrence noted near the other tedi-search in
the same story).
In `@tedi/components/layout/sidenav/sidenav-toggle/sidenav-toggle.component.scss`:
- Around line 15-17: Replace the element selector styling for tedi-icon with a
class-based selector and add that class to the component template/host;
specifically, remove the direct selector "tedi-icon { ... }" in
sidenav-toggle.component.scss and use a dedicated class (e.g.,
.sidenav-toggle__icon) so the rule targets .sidenav-toggle__icon { color:
var(--button-main-primary-text-default); }, and update the sidenav-toggle
component template/host to apply that class to the tedi-icon element or host
element so the style continues to apply.
In `@tedi/components/overlay/popover/popover.component.scss`:
- Around line 8-11: Replace the element selector styling "tedi-popover { ... }"
with a host class selector (e.g. ".tedi-popover { display: inline-flex;
align-items: center; }") and ensure the Popover component adds that class to its
host (add host: { class: 'tedi-popover' } in the `@Component` decorator or add the
class to the host element). Update popover.component.scss to target
".tedi-popover" instead of the element selector and add the host class in the
popover component (e.g., PopoverComponent) so the styles apply via the host
class.
---
Outside diff comments:
In
`@tedi/components/layout/header/header-language/header-language.component.spec.ts`:
- Around line 25-31: The TestBed setup for HeaderLanguageComponent is missing
the TEDI_TRANSLATION_DEFAULT_TOKEN provider; update the providers array in the
TestBed.configureTestingModule call to include { provide:
TEDI_TRANSLATION_DEFAULT_TOKEN, useValue: /* default token value or mock */ }
alongside the existing { provide: TediTranslationService, useValue:
mockTranslationService } so the translated component has the required default
token during tests.
In
`@tedi/components/layout/header/header-profile/header-profile.component.spec.ts`:
- Around line 15-22: Add a mock provider for TediTranslationService to the
TestBed.configureTestingModule providers list alongside the existing
TEDI_TRANSLATION_DEFAULT_TOKEN so the component has a predictable translation
service instance; update the providers array to include a mock object (or
jasmine.createSpyObj) for TediTranslationService and remove or simplify per-test
spies that access component.translationService (e.g., the spy in the
resolvedLabel test) so tests use the injected mock consistently.
---
Nitpick comments:
In `@tedi/components/layout/header/header-login/header-login.component.spec.ts`:
- Line 68: Replace the direct property assignment to the test component instance
(fixture.componentInstance.label = "Sign in") with the consistent test API by
calling fixture.componentRef.setInput('label', 'Sign in'); locate the occurrence
in header-login.component.spec.ts where fixture.componentInstance is used and
change it to use fixture.componentRef.setInput so it matches other tests that
set inputs via setInput.
In `@tedi/components/layout/header/header-logout/header-logout.component.ts`:
- Line 37: The BreakpointService instance assigned to breakpointService should
be made private and readonly for consistency and encapsulation: change the
injected property breakpointService (type BreakpointService) to private
readonly, similar to translationService, since breakpointService is only used
internally within HeaderLogoutComponent; update any references accordingly if
needed.
In `@tedi/components/layout/header/header-profile/header-profile.component.ts`:
- Around line 110-116: resolvedLabel returns a fallback translation for mobile
use but the desktop template ignores it, so rename the computed property or
document its intent: update the computed property name from resolvedLabel to
mobileLabel (and update all template and TS references) OR add a brief JSDoc
above resolvedLabel clarifying “used only by the mobile button; desktop uses
label() directly,” and ensure translationService.translate("header.profile")
remains the fallback; reference the computed symbol resolvedLabel/mobileLabel,
the label() accessor, and translationService.translate when making the change.
In `@tedi/components/layout/header/header-role/header-role.component.ts`:
- Around line 140-144: The effect callback currently uses setTimeout(() =>
this.searchInput()?.nativeElement.focus()) to wait for the input to be rendered;
replace that timing hack with Angular's afterNextRender to run the focus after
the next render cycle. Import afterNextRender from `@angular/core` and inside
effect() where you check this.popover()?.isOpen() && this.showInput(), call
afterNextRender(() => this.searchInput()?.nativeElement.focus()) instead of
setTimeout; keep the same null-safe calls to popover, showInput and searchInput
so behavior is preserved.
In `@tedi/components/layout/header/header.component.scss`:
- Line 3: The CSS uses a fallback in the var() call for the box-shadow: replace
the fallback usage var(--tedi-alpha-20, rgb(0 0 0 / 20%)) with a bare variable
reference var(--tedi-alpha-20) so the SCSS rule is satisfied; update the
box-shadow declaration that references --tedi-alpha-20 to remove the fallback
and ensure the design token is defined elsewhere.
🪄 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: 919bfca0-677b-4f71-9cc6-240126019107
⛔ Files ignored due to path filters (1)
public/header-logo-white.svgis excluded by!**/*.svg
📒 Files selected for processing (57)
.storybook/preview.tsxtedi/components/base/text/text.component.tstedi/components/layout/header/header-actions/header-actions.component.scsstedi/components/layout/header/header-bottom/header-bottom.component.scsstedi/components/layout/header/header-bottom/header-bottom.component.spec.tstedi/components/layout/header/header-bottom/header-bottom.component.tstedi/components/layout/header/header-bottom/index.tstedi/components/layout/header/header-content/header-content.component.scsstedi/components/layout/header/header-content/header-content.component.spec.tstedi/components/layout/header/header-content/header-content.component.tstedi/components/layout/header/header-language/header-language.component.htmltedi/components/layout/header/header-language/header-language.component.scsstedi/components/layout/header/header-language/header-language.component.spec.tstedi/components/layout/header/header-login/header-login.component.htmltedi/components/layout/header/header-login/header-login.component.scsstedi/components/layout/header/header-login/header-login.component.spec.tstedi/components/layout/header/header-login/header-login.component.tstedi/components/layout/header/header-logo/header-logo-dark.directive.tstedi/components/layout/header/header-logo/header-logo.component.htmltedi/components/layout/header/header-logo/header-logo.component.scsstedi/components/layout/header/header-logo/header-logo.component.spec.tstedi/components/layout/header/header-logo/header-logo.component.tstedi/components/layout/header/header-logout/header-logout.component.htmltedi/components/layout/header/header-logout/header-logout.component.scsstedi/components/layout/header/header-logout/header-logout.component.spec.tstedi/components/layout/header/header-logout/header-logout.component.tstedi/components/layout/header/header-mobile-button/header-mobile-button.component.htmltedi/components/layout/header/header-mobile-button/header-mobile-button.component.scsstedi/components/layout/header/header-mobile-button/header-mobile-button.component.spec.tstedi/components/layout/header/header-mobile-button/header-mobile-button.component.tstedi/components/layout/header/header-mobile-button/index.tstedi/components/layout/header/header-profile/header-profile.component.htmltedi/components/layout/header/header-profile/header-profile.component.scsstedi/components/layout/header/header-profile/header-profile.component.spec.tstedi/components/layout/header/header-profile/header-profile.component.tstedi/components/layout/header/header-role/header-role-title.directive.tstedi/components/layout/header/header-role/header-role.component.htmltedi/components/layout/header/header-role/header-role.component.scsstedi/components/layout/header/header-role/header-role.component.spec.tstedi/components/layout/header/header-role/header-role.component.tstedi/components/layout/header/header-search/header-search.component.htmltedi/components/layout/header/header-search/header-search.component.scsstedi/components/layout/header/header-search/header-search.component.spec.tstedi/components/layout/header/header-search/header-search.component.tstedi/components/layout/header/header-search/index.tstedi/components/layout/header/header.component.htmltedi/components/layout/header/header.component.scsstedi/components/layout/header/header.stories.tstedi/components/layout/header/index.tstedi/components/layout/sidenav/sidenav-item/sidenav-item.component.scsstedi/components/layout/sidenav/sidenav-toggle/sidenav-toggle.component.htmltedi/components/layout/sidenav/sidenav-toggle/sidenav-toggle.component.scsstedi/components/navigation/link/link.component.scsstedi/components/overlay/popover/popover-trigger/popover-trigger.directive.tstedi/components/overlay/popover/popover.component.scsstedi/components/overlay/popover/popover.component.tstedi/services/translation/translations.ts
💤 Files with no reviewable changes (1)
- tedi/components/layout/header/header-login/header-login.component.scss
Comment on lines
+54
to
+92
| /** | ||
| * Custom label text for the profile button. When provided, used as-is — not | ||
| * translated. When omitted or empty, the desktop trigger renders as an | ||
| * icon-only button (no visible label) and the mobile trigger falls back to | ||
| * the `header.profile` translation key. | ||
| */ | ||
| label = input(""); | ||
| /** | ||
| * Defines the breakpoint from which the profile menu is displayed as a popover. | ||
| * Below this breakpoint, it is rendered as a modal. | ||
| * | ||
| * @default 'lg' | ||
| */ | ||
| showPopover = input<Breakpoint>("lg"); | ||
|
|
||
| /** | ||
| * Removes default item styles from the mobile modal content. When `true`, | ||
| * projected children render without the padding, border, or background that | ||
| * `<tedi-header-profile>` normally applies to each direct child of the | ||
| * modal. Use when the content requires custom item styling. | ||
| * | ||
| * Only affects the mobile/modal branch. The desktop popover uses | ||
| * `<tedi-popover-content>`'s own styling and is unaffected. | ||
| * | ||
| * @default false | ||
| */ | ||
| noStyle = input<boolean>(false); | ||
|
|
||
| /** | ||
| * Visual size of the profile button. | ||
| * - `'small'` renders a compact icon-and-caption button (the | ||
| * `<tedi-header-mobile-button>` variant). | ||
| * - `'default'` renders a button with the user's name + icon. | ||
| * | ||
| * When left unset, the size is chosen automatically based on the viewport: | ||
| * `'small'` below the `md` breakpoint, `'default'` from `md` up. Pass an | ||
| * explicit value to force a variant regardless of viewport. | ||
| */ | ||
| size = input<HeaderProfileSize>(); |
There was a problem hiding this comment.
Document the breaking changes in the public API.
The component's input API has significantly changed:
- Removed:
nameinput - Removed:
showDropdowninput - Added:
label,showPopover,noStyle,sizeinputs
These are breaking changes for consumers. Ensure they are documented in release notes or migration guides.
🤖 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/layout/header/header-profile/header-profile.component.ts`
around lines 54 - 92, Update the public changelog and migration guide to call
out the input API changes for HeaderProfileComponent: state that the inputs name
and showDropdown were removed and list the new inputs label, showPopover,
noStyle, and size (reference the component inputs label, showPopover, noStyle,
size in header-profile.component.ts); provide a short migration snippet and
recommended replacements (e.g., replace name with label or elsewhere, replace
showDropdown logic with showPopover/modal behavior) and mark this as a breaking
change in the release notes with the component name and version bump.
Comment on lines
39
to
55
| export type Representative = { | ||
| /** | ||
| * Stable identifier. Required for selection comparison and for re-syncing | ||
| * the current representative when the `representatives` input changes. | ||
| */ | ||
| id: string; | ||
| name: string; | ||
| icon?: string; | ||
| /** | ||
| * Icon shown next to the representative. Accepts either a Material Icon name | ||
| * as a string (`'person'`) for the common case, or a full | ||
| * `RepresentativeIcon` object (`{ name: 'person', size: 18 }`) when an | ||
| * explicit size is needed. The two forms are equivalent when only `name` is | ||
| * relevant. | ||
| */ | ||
| icon?: string | RepresentativeIcon; | ||
| description?: string; | ||
| }; |
There was a problem hiding this comment.
Breaking change: Representative.id is now required.
The Representative type now requires an id field. This is a breaking change for consumers who may be passing representative objects without an id. Ensure this change is documented in release notes or migration guides.
🤖 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/layout/header/header-role/header-role.component.ts` around
lines 39 - 55, Representative.id was made required which breaks callers; update
documentation and add a runtime compatibility fix: document the breaking change
in release notes/migration guide and, in the component that consumes the type
(e.g., HeaderRoleComponent handling the representatives input and selection
logic such as the code paths that sync selection or call
setSelectedRepresentative), detect missing id values and synthesize a stable id
(for example from name or index or a UUID) before using them for
comparison/selection, or revert the type to keep id optional until consumers are
migrated. Ensure references to the Representative type and its id field are
updated accordingly.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/layout/header/header-login/header-login.component.spec.ts`:
- Around line 40-45: The test setup for HeaderLoginComponent configures a mocked
TediTranslationService but omits providing TEDI_TRANSLATION_DEFAULT_TOKEN; add a
provider entry { provide: TEDI_TRANSLATION_DEFAULT_TOKEN, useValue: <appropriate
default> } to the TestBed.configureTestingModule providers alongside the mocked
TediTranslationService (and ensure the token import is present) so the
translated component test follows repo conventions.
In `@tedi/components/layout/header/header-role/header-role.component.ts`:
- Line 2: The import of the private CDK symbol `_IdGenerator` must be removed
and replaced with a stable ID generation approach: remove `_IdGenerator` usage
in header-role.component.ts and implement either a simple module-level counter
or a small injectable service (e.g., HeaderIdService or a file-level counter
like nextHeaderRoleId) to produce deterministic IDs for the component; update
any usages of `_IdGenerator` in the component class (e.g., constructor or
methods that call it) to call your new generator service/function and ensure IDs
remain unique across instances.
🪄 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: 8bf5ec04-87ff-45e1-affd-b2ec9076cdd8
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (26)
package.jsontedi/components/layout/header/header-language/header-language.component.htmltedi/components/layout/header/header-language/header-language.component.scsstedi/components/layout/header/header-language/header-language.component.spec.tstedi/components/layout/header/header-login/header-login.component.htmltedi/components/layout/header/header-login/header-login.component.spec.tstedi/components/layout/header/header-login/header-login.component.tstedi/components/layout/header/header-logout/header-logout.component.spec.tstedi/components/layout/header/header-logout/header-logout.component.tstedi/components/layout/header/header-profile/header-profile.component.htmltedi/components/layout/header/header-profile/header-profile.component.scsstedi/components/layout/header/header-profile/header-profile.component.spec.tstedi/components/layout/header/header-profile/header-profile.component.tstedi/components/layout/header/header-role/header-role.component.htmltedi/components/layout/header/header-role/header-role.component.scsstedi/components/layout/header/header-role/header-role.component.spec.tstedi/components/layout/header/header-role/header-role.component.tstedi/components/layout/header/header-search/header-search.component.spec.tstedi/components/layout/header/header-search/header-search.component.tstedi/components/layout/header/header.component.scsstedi/components/layout/header/header.stories.tstedi/components/layout/sidenav/sidenav-toggle/sidenav-toggle.component.htmltedi/components/layout/sidenav/sidenav-toggle/sidenav-toggle.component.scsstedi/components/overlay/popover/popover.component.scsstedi/components/overlay/popover/popover.component.tstedi/services/translation/translations.ts
✅ Files skipped from review due to trivial changes (5)
- package.json
- tedi/components/layout/sidenav/sidenav-toggle/sidenav-toggle.component.scss
- tedi/components/layout/sidenav/sidenav-toggle/sidenav-toggle.component.html
- tedi/components/layout/header/header.component.scss
- tedi/components/layout/header/header-language/header-language.component.html
🚧 Files skipped from review as they are similar to previous changes (15)
- tedi/components/layout/header/header-profile/header-profile.component.spec.ts
- tedi/components/layout/header/header-language/header-language.component.spec.ts
- tedi/components/layout/header/header-language/header-language.component.scss
- tedi/components/overlay/popover/popover.component.scss
- tedi/components/layout/header/header-login/header-login.component.ts
- tedi/components/layout/header/header-search/header-search.component.spec.ts
- tedi/components/layout/header/header-logout/header-logout.component.spec.ts
- tedi/services/translation/translations.ts
- tedi/components/layout/header/header-role/header-role.component.spec.ts
- tedi/components/layout/header/header-logout/header-logout.component.ts
- tedi/components/layout/header/header-role/header-role.component.html
- tedi/components/layout/header/header-search/header-search.component.ts
- tedi/components/layout/header/header-profile/header-profile.component.scss
- tedi/components/layout/header/header-role/header-role.component.scss
- tedi/components/layout/header/header.stories.ts
ly-tempel-bitweb
temporarily deployed
to
github-pages
GitHub Actions
Inactive
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Migration guide
Breaking changes
HeaderRoleComponent—roleinput renamed tolabelThe input that previously held the role text (e.g.
"Roll:") is now calledlabelto align with React'sHeader.RoleAPI and to free theroleattribute from being reflected onto the host element (axe was flagging non-ARIA values like"Asutus"/"Roll:"). The content-projection slot for richer markup (e.g. a<tedi-tag>) is unchanged:[tedi-header-role-title].HeaderRoleComponent—Representative.idis now requiredEach representative must carry a stable
id. It's used for selection comparison and for re-syncing the current representative when therepresentativeslist changes. The previous shape (onlyname/description/ optionalicon) won't compile.The
Representative.icontype also widened fromstringtostring | RepresentativeIcon— that one is additive (the oldicon: 'person'form still works, you can now also pass{ name: 'person', size: 18 }).HeaderProfileComponent—nameinput renamed tolabelFor the same reason as
HeaderRoleComponentand for parity with React.HeaderProfileComponent—showDropdowninput renamed toshowPopover, default changedThe Breakpoint input controlling the desktop popover threshold is now
showPopover(matching React) and defaults to'lg'instead ofundefined.HeaderLogoutComponent— content projection removed; uselabelinput insteadThe component previously rendered projected children inside its label span. It's now input-driven (matching React) and falls back to translation keys when no label is provided.
Translation key rename:
header.login-mobile→header.login.mobileThe mobile-variant translation keys now use a dot separator:
header.login.mobile,header.logout.mobile,header.profile.mobile. Consumers with custom Estonian/English/Russian overrides forheader.login-mobile(Angular pre-PR) need to rename toheader.login.mobile. The bundled values are unchanged.Translation value change:
header.profileThe bundled value changed from
Profiil/Profile/ПрофильtoMinu profiil/My profile/Мой профильto match React. A new keyheader.profile.mobilewas added with the old short values for the compact mobile trigger. Consumers who ship their own translation overrides forheader.profileshould review whether they want the longer or shorter text — the rename below is automatic in the bundled translations, but custom overrides keep their existing values.HeaderLoginmobile breakpoint shifted fromsmtomdThe compact/icon-only login variant now activates below the
mdbreakpoint instead of belowsm. Visual change rather than API change — verify any responsive design that depended on the old threshold.Not breaking — worth knowing
HeaderLogoComponent,HeaderBottomComponent,HeaderMobileButtonComponent, plus directivesHeaderLogoDarkDirective(tediHeaderLogoDarkslot) andHeaderRoleTitleDirective([tedi-header-role-title]slot).HeaderRoleComponent.roleSelectionToggle(firesbooleanwhen the role popover/mobile collapse opens or closes; complements the existingcurrentRepresentativeChangefrommodel.required<Representative>()).HeaderLoginComponentandHeaderLogoutComponent:size,label,href.HeaderContentComponentgained analignmentinput ('flex-start' | 'center' | 'space-between', default'center'); the default ofcentermay visually shift content compared to the previous no-flex template.HeaderProfile,HeaderLogin,HeaderLogoutvia the existingBreakpointInputs<T>pattern. Each acceptsxs / sm / md / lg / xl / xxlinputs that overridelabel/size/showPopoverat the matching breakpoint:LinkComponentalready uses, mirrors React'sBreakpointSupport<T>.aria-controls=""axe violation on every[tedi-popover-trigger](HeaderProfile, HeaderLanguage, HeaderRole popovers). The attribute is now omitted until the floating-UI container has a real id, instead of binding to an empty string.Summary by CodeRabbit
New Features
Bug Fixes
Style & UX Improvements