feat(nav-menu): [nav-menu] Add custom selected mode #2718
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant User
participant NavMenu
participant RenderlessLogic
User->>NavMenu: Set defaultActive prop
NavMenu->>RenderlessLogic: Initialize defaultActiveId
RenderlessLogic-->>NavMenu: Update selection state
NavMenu->>User: Render menu with selected item
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (11)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Walkthrough: This PR introduces a new feature to the nav-menu component, allowing customization of the selected menu item through a new Changes:
|
![petercat-assistant[bot]](https://avatars.githubusercontent.com/in/875736?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (11)
examples/sites/demos/pc/app/nav-menu/selecte.spec.ts (1)
3-12: Consider adding more assertions for comprehensive test coverage.
Currently, this test verifies the presence of a default selected item and checks hover behavior. You could further assert that clicking on the item triggers any relevant event or navigation, ensuring end-to-end functionality is covered.packages/vue/src/nav-menu/src/index.ts (1)
26-26: Check whether a default value or more specific type would be beneficial.
While theStringtype is valid, consider providing a default value or refining it to ensure type safety, especially if you expect only certain kinds of identifiers or an empty string.packages/renderless/src/nav-menu/vue.ts (2)
95-95: InitializedefaultActiveIdto track default selected item
DeclaringdefaultActiveIdasnullis a clean approach. Make sure to handle the scenario where no default ID is provided. Also verify that changes inprops.defaultActivewill correctly reflect indefaultActiveId.
152-155: Improve clarity for selection methods
Passingstateinto each of these methods is straightforward, but ensure correct usage of references to submenus vs. main menu items. Consider extracting shared logic if these methods share significant code to maintain simpler code in each.examples/sites/demos/pc/app/nav-menu/webdoc/nav-menu.js (1)
83-94: Document the new “selecte” demo
The new demo entry showcases how to customize the active menu item withdefault-active. This is a valuable example for users. Make sure to keep the naming consistent—“selecte” might be a typo. Consider renaming it to “selected” for clarity.- demoId: 'selecte', + demoId: 'selected',packages/vue/src/nav-menu/src/pc.vue (1)
184-185: Use new props carefully
allowFullUrlanddefaultActiveexpand functionality. Be sure to document them in the component’s official docs or JSDoc for future maintainers and end-users.examples/sites/demos/pc/app/nav-menu/selecte-composition-api.vue (1)
13-369: Comprehensive hierarchical data
This nested menu structure thoroughly tests the new selection logic. For maintainability, consider extracting some of this large data into a separate file or composable to reduce clutter in the demo component.examples/sites/demos/pc/app/nav-menu/selecte.vue (2)
1-5: File name clarification.The file name "selecte.vue" might be slightly confusing—consider renaming it to something like "select.vue" or "selected.vue" to accurately reflect its purpose and avoid potential confusion.
16-371: Consider moving large menu data to a separate file or config.The deeply nested
menuDataobject enhances clarity but may impact maintainability. For large or dynamically managed menus, consider externalizing the data into a separate JSON/config file or a dedicated state management solution to improve readability and reusability.packages/renderless/src/nav-menu/index.ts (2)
532-532: Avoid magic number usage.Using hard-coded
90for available menu width can be confusing. Extract it into a named constant (e.g.,RESERVED_WIDTH) or make it a prop so future adjustments are more maintainable.
648-662: Short-circuit to improve performance.The
getSelectedStatefunction checks each data element but doesn’t exit early upon finding a match. Consider returning immediately whenisSelectedis set totrue, which could save cycles for large menu data.const getSelectedState = (itemData, id) => { let isSelected = false for (const data of itemData) { if (data?.id === id) { return true } else if (data?.children?.length) { for (const dataChildren of data.children) { if (dataChildren?.id === id) { - isSelected = true - } - }) - } - }) - return isSelected + return true + } + } } } + return false }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
examples/sites/demos/apis/nav-menu.js(1 hunks)examples/sites/demos/pc/app/nav-menu/selecte-composition-api.vue(1 hunks)examples/sites/demos/pc/app/nav-menu/selecte.spec.ts(1 hunks)examples/sites/demos/pc/app/nav-menu/selecte.vue(1 hunks)examples/sites/demos/pc/app/nav-menu/webdoc/nav-menu.js(1 hunks)packages/mobile/types/nav-menu.type.ts(1 hunks)packages/renderless/src/nav-menu/index.ts(4 hunks)packages/renderless/src/nav-menu/vue.ts(4 hunks)packages/theme/src/nav-menu/index.less(3 hunks)packages/vue/src/nav-menu/src/index.ts(1 hunks)packages/vue/src/nav-menu/src/pc.vue(5 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/renderless/src/nav-menu/index.ts
[error] 614-617: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
[error] 626-629: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
[error] 637-640: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
[error] 645-648: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
🔇 Additional comments (19)
examples/sites/demos/pc/app/nav-menu/selecte.spec.ts (1)
1-2: Imports look good.
The Playwright test framework is correctly imported here.
packages/mobile/types/nav-menu.type.ts (1)
50-50: Validate whether both 'string | null' are necessary.
If your application logic allows for an unselected state, null might be appropriate. Otherwise, consider defaulting to an empty string to avoid null checks in usage sites.
examples/sites/demos/apis/nav-menu.js (1)
81-94: Documentation clarity is solid.
The new default-active property is well-described, including usage details and version stability. Ensure consumers understand how to configure data to match the ID for a seamless experience.
packages/renderless/src/nav-menu/vue.ts (2)
51-54: Enhance unit tests for new selection methods
These newly added methods (getMoreSelected, getTabSelected, getLeftSelected, and getLastChildSelected) look good in principle. However, ensure they are thoroughly tested, particularly for edge cases such as empty data, deeply nested items, and invalid indexes.
81-84: Include new methods in the public API
Adding getMoreSelected, getLeftSelected, getLastChildSelected, and getTabSelected to the public API (api array) aligns with the new feature requirements. Ensure that each method’s usage is documented clearly so that developers can utilize these methods effectively.
packages/vue/src/nav-menu/src/pc.vue (4)
34-34: Move away from index-based selection
Using getTabSelected(item, index) is more robust than comparing index to state.selectedIndex. This helps future-proof the selection logic. Double-check that any references to state.selectedIndex elsewhere in the code are either updated or remain compatible.
46-49: Display logic for “more” menu
Tying the .selected class to state.enterMoreMenu || getMoreSelected() is more flexible. Ensure that enterMoreMenu is reset appropriately on mouse leave events and that getMoreSelected() returns the correct result even when no item is selected.
75-78: Use getLeftSelected consistently
Switching from direct index checks to getLeftSelected(item, index) aligns with the new approach. Confirm that custom logic doesn’t revert back to index-based checks, especially where submenus are also manipulated.
135-137: Leverage getLastChildSelected for sub-items
Replacing multiple nested index conditions with a single function call improves readability and consistency. Ensure child vs. grandchild-level selections are properly distinguished in getLastChildSelected().
examples/sites/demos/pc/app/nav-menu/selecte-composition-api.vue (2)
1-3: Demonstrate defaultActive usage
Providing an example with :defaultActive="defaultActive" is extremely helpful. The code is clear and concise.
11-11: Ensure correctness of default active ID
The defaultActive ref is set to '2-1-1'. Make sure this ID exists in menuData to avoid referencing a non-existent item. Provide examples or fallback logic if the ID can’t be found.
examples/sites/demos/pc/app/nav-menu/selecte.vue (2)
7-7: Confirm JSX usage.
You’ve specified lang="jsx" in the <script> block. If you’re intentionally using the Vue JSX transform, that’s fine. Otherwise, confirm that your build chain supports Vue + JSX to avoid compilation issues.
377-385: Scoped styles look good.
Your scoped styles effectively define the .preview container styling while ensuring a consistent look for the navigation menu. They appear concise and well-organized.
packages/renderless/src/nav-menu/index.ts (2)
155-157: Default active ID initialization.
Initializing state.defaultActiveId from props.defaultActive here cleanly aligns with the new property’s intended usage. This looks correct.
443-445: Avoid overwriting defaultActiveId unnecessarily.
This mirrors prior feedback about ensuring state.defaultActiveId is only reassigned when truly required. Consider confirming with a check:
if (state.defaultActiveId) {
- state.defaultActiveId = item.id
+ if (!item.id || item.id !== state.defaultActiveId) {
+ state.defaultActiveId = item.id
+ }
}packages/theme/src/nav-menu/index.less (4)
31-31: Switch to padding is acceptable.
This modification provides a more controlled spacing around the logo. Ensure it aligns with the overall layout design.
100-104: Consistent hover enhancements.
Eliminating background color in favor of bold text and consistent text color simplifies the UI. This is a tidy style improvement.
109-109: Full-width underline for .selected.
Expanding the underline to width: 100% ensures a uniform highlight. The added styles for border radius and weight appear consistent with the rest of the menu’s design.
Also applies to: 113-113, 116-117
163-163: .active, &.selected, &:hover synergy.
Combining .active with .selected and :hover states under a single block clarifies the styling rules, reducing code duplication. This is a neat consolidation.
| export const getLastChildSelected = | ||
| ({ state }: Pick<INavMenuRenderlessParams, 'state'>) => | ||
| (item, i, index): void => { | ||
| let isSelected = | ||
| item?.id === state.defaultActiveId || (i === state.subItemSelectedIndex && index === state.subIndex) | ||
| return isSelected | ||
| } | ||
|
|
There was a problem hiding this comment.
Return type mismatch in getLastChildSelected.
Same pattern: change the declared return type to boolean if you need to return a boolean value.
-export const getLastChildSelected =
- ({ state }: Pick<INavMenuRenderlessParams, 'state'>) =>
- (item, i, index): void => {
+export const getLastChildSelected =
+ ({ state }: Pick<INavMenuRenderlessParams, 'state'>) =>
+ (item, i, index): boolean => {
// ...
return isSelected
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getLastChildSelected = | |
| ({ state }: Pick<INavMenuRenderlessParams, 'state'>) => | |
| (item, i, index): void => { | |
| let isSelected = | |
| item?.id === state.defaultActiveId || (i === state.subItemSelectedIndex && index === state.subIndex) | |
| return isSelected | |
| } | |
| export const getLastChildSelected = | |
| ({ state }: Pick<INavMenuRenderlessParams, 'state'>) => | |
| (item, i, index): boolean => { | |
| let isSelected = | |
| item?.id === state.defaultActiveId || (i === state.subItemSelectedIndex && index === state.subIndex) | |
| return isSelected | |
| } |
| export const getMoreSelected = | ||
| ({ state }: Pick<INavMenuRenderlessParams, 'state'>) => | ||
| (): void => { | ||
| let isSelected = false | ||
| if (state?.more.length) { | ||
| state.more.forEach((item) => { | ||
| if (item?.id === state.defaultActiveId) { | ||
| isSelected = true | ||
| } else if (item?.children?.length) { | ||
| isSelected = getSelectedState(item.children, state.defaultActiveId) | ||
| } | ||
| }) | ||
| } | ||
| return isSelected | ||
| } |
There was a problem hiding this comment.
Return type mismatch in getMoreSelected.
The function signature says (): void =>, but it returns a boolean. Align the return type to avoid potential type/lint errors:
-export const getMoreSelected =
- ({ state }: Pick<INavMenuRenderlessParams, 'state'>) =>
- (): void => {
+export const getMoreSelected =
+ ({ state }: Pick<INavMenuRenderlessParams, 'state'>) =>
+ (): boolean => {
// ...
return isSelected
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getMoreSelected = | |
| ({ state }: Pick<INavMenuRenderlessParams, 'state'>) => | |
| (): void => { | |
| let isSelected = false | |
| if (state?.more.length) { | |
| state.more.forEach((item) => { | |
| if (item?.id === state.defaultActiveId) { | |
| isSelected = true | |
| } else if (item?.children?.length) { | |
| isSelected = getSelectedState(item.children, state.defaultActiveId) | |
| } | |
| }) | |
| } | |
| return isSelected | |
| } | |
| export const getMoreSelected = | |
| ({ state }: Pick<INavMenuRenderlessParams, 'state'>) => | |
| (): boolean => { | |
| let isSelected = false | |
| if (state?.more.length) { | |
| state.more.forEach((item) => { | |
| if (item?.id === state.defaultActiveId) { | |
| isSelected = true | |
| } else if (item?.children?.length) { | |
| isSelected = getSelectedState(item.children, state.defaultActiveId) | |
| } | |
| }) | |
| } | |
| return isSelected | |
| } |
| export const getTabSelected = | ||
| ({ state }: Pick<INavMenuRenderlessParams, 'state'>) => | ||
| (item, index): void => { | ||
| let isChildSelected = false | ||
| if (item?.id && item?.children?.length) { | ||
| isChildSelected = getSelectedState(item.children, state.defaultActiveId) | ||
| } | ||
| let isSelected = item?.id === state.defaultActiveId || isChildSelected || index === state.selectedIndex | ||
|
|
||
| return isSelected | ||
| } |
There was a problem hiding this comment.
Return type mismatch in getTabSelected.
Same issue—function returns a boolean while declared as void. Change the return type accordingly for correctness.
-export const getTabSelected =
- ({ state }: Pick<INavMenuRenderlessParams, 'state'>) =>
- (item, index): void => {
+export const getTabSelected =
+ ({ state }: Pick<INavMenuRenderlessParams, 'state'>) =>
+ (item, index): boolean => {
// ...
return isSelected
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getTabSelected = | |
| ({ state }: Pick<INavMenuRenderlessParams, 'state'>) => | |
| (item, index): void => { | |
| let isChildSelected = false | |
| if (item?.id && item?.children?.length) { | |
| isChildSelected = getSelectedState(item.children, state.defaultActiveId) | |
| } | |
| let isSelected = item?.id === state.defaultActiveId || isChildSelected || index === state.selectedIndex | |
| return isSelected | |
| } | |
| export const getTabSelected = | |
| ({ state }: Pick<INavMenuRenderlessParams, 'state'>) => | |
| (item, index): boolean => { | |
| let isChildSelected = false | |
| if (item?.id && item?.children?.length) { | |
| isChildSelected = getSelectedState(item.children, state.defaultActiveId) | |
| } | |
| let isSelected = item?.id === state.defaultActiveId || isChildSelected || index === state.selectedIndex | |
| return isSelected | |
| } |
| export const getLeftSelected = | ||
| ({ state }: Pick<INavMenuRenderlessParams, 'state'>) => | ||
| (item, index): void => { | ||
| let isLeftChildSelected = false | ||
| if (item?.id && item?.children?.length) { | ||
| isLeftChildSelected = getSelectedState(item.children, state.defaultActiveId) | ||
| } | ||
| let isSelected = item?.id === state.defaultActiveId || isLeftChildSelected || index === state.moreItemSelectedIndex | ||
| return isSelected | ||
| } |
There was a problem hiding this comment.
Return type mismatch in getLeftSelected.
Again, the function returns a boolean but is typed as void. Adjust the return type to ensure consistency.
-export const getLeftSelected =
- ({ state }: Pick<INavMenuRenderlessParams, 'state'>) =>
- (item, index): void => {
+export const getLeftSelected =
+ ({ state }: Pick<INavMenuRenderlessParams, 'state'>) =>
+ (item, index): boolean => {
// ...
return isSelected
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getLeftSelected = | |
| ({ state }: Pick<INavMenuRenderlessParams, 'state'>) => | |
| (item, index): void => { | |
| let isLeftChildSelected = false | |
| if (item?.id && item?.children?.length) { | |
| isLeftChildSelected = getSelectedState(item.children, state.defaultActiveId) | |
| } | |
| let isSelected = item?.id === state.defaultActiveId || isLeftChildSelected || index === state.moreItemSelectedIndex | |
| return isSelected | |
| } | |
| export const getLeftSelected = | |
| ({ state }: Pick<INavMenuRenderlessParams, 'state'>) => | |
| (item, index): boolean => { | |
| let isLeftChildSelected = false | |
| if (item?.id && item?.children?.length) { | |
| isLeftChildSelected = getSelectedState(item.children, state.defaultActiveId) | |
| } | |
| let isSelected = item?.id === state.defaultActiveId || isLeftChildSelected || index === state.moreItemSelectedIndex | |
| return isSelected | |
| } |
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
default-activeproperty to customize the currently selected menu itemDocumentation
Tests
Style