fix(navbar): errors following a full review of the component (#DS-5403) - #1915
Open
lskramarov wants to merge 3 commits into
Open
fix(navbar): errors following a full review of the component (#DS-5403)#1915lskramarov wants to merge 3 commits into
lskramarov wants to merge 3 commits into
Conversation
|
Visit the preview URL for this PR (updated for commit 430c54c): https://koobiq-next--prs-1915-ppoj71wr.web.app (expires Sat, 22 Aug 2026 08:09:27 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: c9e37e518febda70d0317d07e8ceb35ac43c534c |
🚨 E2E tests failedReview the report for details. 💡 Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR completes a follow-up “full review” pass on the @koobiq/components/navbar package by migrating remaining inputs to signal-based APIs, tightening accessibility semantics (ARIA roles/names/disabled state), adjusting layering/sizing via CSS tokens, and adding an ng update migration to help consumers adapt to the API surface changes.
Changes:
- Refactors navbar components toward signal-based APIs and tooltip composition (vs inheritance), including updated ARIA semantics and keyboard interactions.
- Introduces a new
navbar-signals-and-ariamigration schematic (auto-fixes signal reads/writes + warns on manual changes) and wires it into schematics collections/migrations. - Updates styles to rely on new CSS tokens for sizing/z-index, plus adds e2e interaction coverage for runtime states (dropdown open, toggle, roving focus).
Reviewed changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/public_api_guard/components/tooltip.api.md | Public API snapshot updated to include setOverlayPanelClass. |
| tools/public_api_guard/components/navbar.api.md | Public API snapshot updated for navbar signal APIs, composition changes, and new exports. |
| packages/schematics/src/migrations/navbar-signals-and-aria/schema.ts | Adds migration schema typings for the new navbar migration. |
| packages/schematics/src/migrations/navbar-signals-and-aria/schema.json | Declares CLI schema/options for the migration. |
| packages/schematics/src/migrations/navbar-signals-and-aria/README.md | Documents migration intent, autofix behavior, and manual steps/warnings. |
| packages/schematics/src/migrations/navbar-signals-and-aria/index.ts | Implements the migration (TS rewrites + template ref rewrites + warnings). |
| packages/schematics/src/migrations/navbar-signals-and-aria/index.spec.ts | Adds unit tests for the migration behavior and warnings. |
| packages/schematics/src/migrations/navbar-signals-and-aria/data.ts | Centralizes patterns/members/warnings/notes used by the migration. |
| packages/schematics/src/migrations.json | Registers the migration for ng update. |
| packages/schematics/src/collection.json | Registers the schematic in the schematics collection. |
| packages/e2e/routes.ts | Adds the new navbar interactions e2e route to the shared e2e app. |
| packages/docs-examples/components/navbar/navbar-vertical/navbar-vertical-example.html | Updates docs example for signal reads (expanded() etc.). |
| packages/docs-examples/components/navbar/navbar-vertical-open-over/navbar-vertical-open-over-example.html | Updates docs example for signal reads (expanded() etc.). |
| packages/docs-examples/components/navbar/navbar-vertical-app-long-name/navbar-vertical-app-long-name-example.html | Updates docs example for signal reads (expanded() etc.). |
| packages/docs-examples/components/navbar/navbar-template-outlet/navbar-template-outlet-example.html | Updates docs example for signal reads (expanded() etc.). |
| packages/components/tooltip/tooltip.component.ts | Adds setOverlayPanelClass to support tooltip composition via hostDirectives. |
| packages/components/navbar/vertical-navbar.scss | Replaces hardcoded sizing/z-index with CSS tokens. |
| packages/components/navbar/vertical-navbar.component.ts | Converts vertical navbar to signals/models, adds ARIA landmark name support, updates collapse/expand behavior. |
| packages/components/navbar/navbar.scss | Replaces overlay z-index dependency with CSS token. |
| packages/components/navbar/navbar.component.ts | Converts base focusable API to signals/models, improves resize handling via runOutsideAngular, adds ARIA landmark name support. |
| packages/components/navbar/navbar.component.spec.ts | Expands tests for new signal APIs, keyboard/ARIA behaviors, and resize debouncing; adds axe coverage. |
| packages/components/navbar/navbar.component.html | Updates test template to provide an aria-label and adds labels for icon-only items. |
| packages/components/navbar/navbar-tokens.scss | Introduces CSS tokens for navbar sizing and layering. |
| packages/components/navbar/navbar-toggle.scss | Replaces hardcoded sizing/z-index with CSS tokens. |
| packages/components/navbar/navbar-toggle.component.ts | Introduces toggle registry to scope Ctrl+/ shortcut, adds ARIA semantics, and updates signal usage. |
| packages/components/navbar/navbar-item.scss | Adjusts shared badge positioning rules and relies on orientation partials. |
| packages/components/navbar/navbar-item.component.ts | Refactors navbar items to compose tooltip triggers, adds ARIA naming/role logic, adds Enter/Space activation for bare items, migrates inputs to signals. |
| packages/components/navbar/navbar-item.component.html | Updates template to call computed signals (showVerticalDropDownAngle() etc.). |
| packages/components/navbar/navbar-divider.scss | Replaces hardcoded divider height with token. |
| packages/components/navbar/navbar-brand.scss | Replaces hardcoded sizing and removes !important via specificity adjustments. |
| packages/components/navbar/navbar-brand.component.ts | Refactors brand to compose tooltip trigger and publish accessible names; adjusts focusability logic for non-anchor brands. |
| packages/components/navbar/e2e.ts | Adds a new interaction-focused e2e fixture route component. |
| packages/components/navbar/e2e.playwright-spec.ts | Adds Playwright interaction coverage (dropdown open, toggle expand/collapse, roving focus, tooltip on collapsed focus). |
| packages/components/navbar/_navbar-theme.scss | Removes !important and clarifies specificity rationale. |
| packages/components/navbar/_navbar-item_vertical.scss | Replaces hardcoded size with token and refines badge positioning for expanded vs collapsed layouts. |
| packages/components/navbar/_navbar-item_horizontal.scss | Replaces hardcoded badge offsets with token-based values. |
| packages/components-dev/navbar/template.html | Updates dev template for signal reads (expanded() etc.). |
Suppressed comments (1)
packages/schematics/src/migrations/navbar-signals-and-aria/index.ts:155
receiverOfusesreceivers.find(...), which prefers the first matching receiver. When receivers are shadowed (e.g., an inner function parameter or local with the same name), this can select the outer receiver and either rewrite the wrong accesses or miss rewrites entirely. Prefer the innermost/most-recent receiver instead.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+131
to
+135
| } else if (ts.isVariableDeclaration(node) && ts.isIdentifier(node.name)) { | ||
| const typeName = navbarTypeName(node.type); | ||
|
|
||
| if (typeName) add(node.name.text, findAncestor(node, isFunctionLike) ?? sourceFile, typeName); | ||
| } |
🚨 E2E tests failedReview the report for details. 💡 Comment |
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
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.
No description provided.