-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(navigation): remove prefix nav for menu and menu-item #6774
feat(navigation): remove prefix nav for menu and menu-item #6774
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! I think this is good to merge into the wip navigation branch. Some questions, can address once merged.
describe("calcite-menu", () => { | ||
it("renders", async () => renders("calcite-menu", { display: "flex" })); | ||
|
||
it("honors hidden attribute", async () => hidden("calcite-menu")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to try adding a11y tests at this point or wait for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a11y for menu-item
and menu
components.
src/components/menu/menu.tsx
Outdated
@@ -36,7 +36,7 @@ export class CalciteNavMenu { | |||
@Prop() label: string; | |||
|
|||
// todo evaluate slotted content and determine if it is a nav menu item, then limit # rendered when auto-collapsing based on width of parent | |||
@Prop({ mutable: true }) minCollapsedItems?; | |||
@Prop() minCollapsedItems: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove this, for the time being, until we add back the auto-collapse functionality?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, removed.
|
||
/** | ||
* @internal | ||
*/ | ||
@Prop({ mutable: true }) subMenuOpen = false; | ||
@Prop({ mutable: true }) open = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is internal - 2 questions - is open
descriptive enough? And, should this be publicly documented (is there a reason a dev user might want to be able to toggle this? I could see it maybe being useful in layout="vertical"
menu use cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i gave a thought about changing it to open
, the reason being consistency. Though it is internal, keeping it consistent will be useful if we choose to make it public in future as you mentioned for layout="vertical"
Related Issue: #6531
Summary
nav-menu
tomenu
andnav-menu-item
tomenu-item
.subMenuOpen
toopen
in menu-item.