Skip to content

Navigation button#65

Open
edlu77 wants to merge 11 commits into
mainfrom
navigation-button
Open

Navigation button#65
edlu77 wants to merge 11 commits into
mainfrom
navigation-button

Conversation

@edlu77
Copy link
Copy Markdown
Collaborator

@edlu77 edlu77 commented Jun 3, 2026

@edlu77 edlu77 requested review from LibbyUX and axdanbol June 3, 2026 22:28
@edlu77 edlu77 self-assigned this Jun 3, 2026
@nx-cloud
Copy link
Copy Markdown
Contributor

nx-cloud Bot commented Jun 3, 2026

View your CI Pipeline Execution ↗ for commit 5a19700

Command Status Duration Result
nx affected -t lint test build build-compodoc b... ✅ Succeeded 1m 16s View ↗
nx-cloud record -- nx format:check ✅ Succeeded 2s View ↗
nx-cloud record -- commitlint --from 279c374f39... ✅ Succeeded <1s View ↗

💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗


☁️ Nx Cloud last updated this comment at 2026-06-05 16:48:01 UTC

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 3, 2026

🚀 Preview Deploy Report

✅ Successfully deployed preview here

Copy link
Copy Markdown
Collaborator

@LibbyUX LibbyUX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @edlu77 ! This looks good. One change request:
Can you please remove the underline from the unselected/enabled state? There is no underline for this specific state. On hover unselected, the underline appears while the user is hovering, but if they move away from the button, the component returns to unselected/enabled with no underline.

Please let me know if you have questions! Thanks so much!

Image

This was linked to issues Jun 4, 2026
@LibbyUX LibbyUX added this to the WPP MVP1 milestone Jun 4, 2026
nx-cloud[bot]

This comment was marked as outdated.

Copy link
Copy Markdown
Member

@axdanbol axdanbol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move the toggle into its own entrypoint

styleUrl: './navigation-toggle.scss',
changeDetection: ChangeDetectionStrategy.OnPush,
host: {
'[attr.selected]': 'selected()',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to a class, i.e. [class.ang-navigation-toggle-selected]

changeDetection: ChangeDetectionStrategy.OnPush,
host: {
'[attr.selected]': 'selected()',
'(click)': 'selected.set(!selected())',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer using the update method. selected.update(s => !s)

outline: solid 0.125rem token-utils.slot(focus-outline-color, $config);
}

:is(&:hover, &:active, &:focus) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use focus-visible instead of focus

outline: solid 0.125rem token-utils.slot(focus-outline-color, $config);
}

:is(&.selected, &:hover, &:active, &:focus) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use focus-visible instead of focus

})
export class Navigation {
/** The link to navigate to */
readonly link = input<AnyLinkCommand | null>(null);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove null, input<AnyLinkCommand>()

})
export class NavigationToggle {
/** Whether the button is currently selected */
readonly selected = model<boolean>(false);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove <boolean>, it is inferred

Comment thread tsconfig.base.json Outdated
"@atlasng/core": ["./libs/core/src/index.ts"],
"@atlasng/design-system": ["./libs/design-system/src/index.ts"],
"@atlasng/design-system/buttons/help": ["./libs/design-system/buttons/help/src/index.ts"],
"@atlasng/design-system/buttons/navigation-category-toggle": [
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update

Copy link
Copy Markdown
Contributor

@nx-cloud nx-cloud Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ The fix from Nx Cloud was applied

We corrected the export path in libs/design-system/buttons/navigation-toggle/src/index.ts from ../../navigation-toggle/src/lib/navigation-toggle to ./lib/navigation-toggle. This fix aligns the entry point with its sibling navigation package and resolves the ng-packagr build failure caused by an unresolvable path in the emitted .d.ts file.

Tip

We verified this fix by re-running design-system:build:production.

Suggested Fix changes
diff --git a/libs/design-system/buttons/navigation-toggle/src/index.ts b/libs/design-system/buttons/navigation-toggle/src/index.ts
index b929bdf..bbdd2d1 100644
--- a/libs/design-system/buttons/navigation-toggle/src/index.ts
+++ b/libs/design-system/buttons/navigation-toggle/src/index.ts
@@ -1 +1 @@
-export { NavigationToggle } from '../../navigation-toggle/src/lib/navigation-toggle';
+export { NavigationToggle } from './lib/navigation-toggle';

🔔 Heads up, your workspace has pending recommendations ↗ to auto-apply fixes for similar failures.

Revert fix via Nx Cloud  

View interactive diff ↗

➡️ This fix was applied by Edward Lu

🎓 Learn more about Self-Healing CI on nx.dev

Co-authored-by: edlu77 <edlu77@users.noreply.github.com>
@LibbyUX LibbyUX self-requested a review June 5, 2026 20:40
Copy link
Copy Markdown
Collaborator

@LibbyUX LibbyUX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@edlu77 - The navigation button is approved!

One question on the navigation toggle button: Is it possible to keep the underline visible when it is selected?

The goal is to see the underline when a menu is open:

Image

Inspiration: Figma's mega menu

Image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add navigation button Add navigation toggle button

3 participants