feat: new Menu implementation #2106
Conversation
|
Preview is ready. |
|
Visual Tests Report is ready. |
amje
force-pushed
the
new-menu
branch
2 times, most recently
from
bae0d3c to
aec7afc
Compare
amje
force-pushed
the
new-menu
branch
4 times, most recently
from
c1a5564 to
bea8851
Compare
amje
force-pushed
the
new-menu
branch
2 times, most recently
from
ccc03b7 to
814a938
Compare
|
@sourcery-ai review |
Reviewer's Guide by SourceryThis pull request introduces a new Sequence diagram for opening a nested menusequenceDiagram
participant User
participant MenuTrigger
participant Menu
participant MenuItem
participant SubMenu
User->MenuTrigger: Clicks
activate MenuTrigger
MenuTrigger->Menu: Sets open state to true
activate Menu
Menu->MenuItem: Renders MenuItem with SubMenu
MenuItem->SubMenu: Renders SubMenu with trigger
SubMenu->MenuTrigger: Renders MenuTrigger
User->MenuTrigger: Hovers over MenuTrigger
activate MenuTrigger
MenuTrigger->SubMenu: Sets open state to true
activate SubMenu
SubMenu-->User: Displays SubMenu
deactivate SubMenu
deactivate MenuTrigger
deactivate Menu
deactivate MenuTrigger
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @amje - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a visual focus state to
Buttonwhen it's used as aMenuTrigger. - The
ListItemViewcomponent now acceptscomponentProps, which are then spread onto the underlying component; this is a good pattern to follow for other components as well.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 2 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return; | ||
| } | ||
|
|
||
| if (typeof onClick === 'function') { | ||
| onClick(e); | ||
| if ( | ||
| typeof onClick === 'function' || | ||
| typeof componentProps?.onClick === 'function' | ||
| ) { | ||
| onClick?.(e); | ||
| componentProps?.onClick?.(e); | ||
| } else if (typeof onCollapseChange === 'function') { |
There was a problem hiding this comment.
question (bug_risk): Double-calling onClick handlers may lead to unexpected behavior.
Both onClick from props and componentProps?.onClick are invoked if provided. Ensure that this dual invocation is intended and won’t result in duplicate side effects, especially in scenarios where both callbacks perform similar actions.
| @@ -266,7 +266,7 @@ LANDING_BLOCK--> | |||
|
|
|||
| `loading` — когда в фоновом режиме выполняются асинхронные процессы. | |||
|
|
|||
| `selected` — когда пользователь может может включить (**Enable**) или отключить (**Disable**) кнопку. | |||
| `selected` — когда пользователь может включить (**Enable**) или отключить (**Disable**) кнопку. | |||
There was a problem hiding this comment.
issue (typo): Repeated word "может"
There seems to be a repetition of the word "может". Consider removing one.
Suggested implementation:
`selected` — когда пользователь может включить (**Enable**) или отключить (**Disable**) кнопку.
Ensure that there are no other occurrences of the duplicated word in other parts of the file.
| // The component is needed to run submenu logic hooks. | ||
| // We get <nodeId> of the Popup using "useFloatingParentNodeId" here | ||
| // and <parentId> from using "useFloatingParentNodeId" outside the Popup. | ||
| function MenuPopupContent({ |
There was a problem hiding this comment.
issue (complexity): Consider extracting the nested hook logic and specialized behaviors into custom hooks and smaller subcomponents to improve readability and maintainability of the component, such as the submenu event management and inline menu item pointer handlers..
Consider extracting some of the nested hook logic and specialized behaviors into their own custom hooks and small subcomponents. For example, extracting the submenu event management in the popup into a custom hook can help isolate the side‐effects and make the main component less dense. Here’s a quick example:
// Create a custom hook for floating tree events
function useMenuTreeEvents({
tree,
nodeId,
parentId,
onRequestClose,
open,
}: {
tree: ReturnType<typeof useFloatingTree> | null;
nodeId: string | null;
parentId: string | null;
onRequestClose: () => void;
open: boolean;
}) {
React.useEffect(() => {
if (!tree || !nodeId) return;
function handleTreeClick() {
if (!parentId) {
onRequestClose();
}
}
function handleSubMenuOpen(event: { nodeId: string; parentId: string }) {
if (event.nodeId !== nodeId && event.parentId === parentId) {
onRequestClose();
}
}
tree.events.on('click', handleTreeClick);
tree.events.on('menuopen', handleSubMenuOpen);
return () => {
tree.events.off('click', handleTreeClick);
tree.events.off('menuopen', handleSubMenuOpen);
};
}, [tree, nodeId, parentId, onRequestClose]);
React.useEffect(() => {
if (open && tree && nodeId) {
tree.events.emit('menuopen', { parentId, nodeId });
}
}, [open, tree, nodeId, parentId]);
}
export function MenuPopupContent(props: /* ... */) {
// destructure props
const { open, onRequestClose, parentId, children, className, style, qa } = props;
const tree = useFloatingTree();
const nodeId = useFloatingParentNodeId();
useMenuTreeEvents({ tree, nodeId, parentId, onRequestClose, open });
return (
<div className={b(null, className)} style={style} data-qa={qa}>
{children}
</div>
);
}Likewise, you might extract and isolate the inline menu item pointer handlers into a separate hook. This separation not only reduces the complexity in the main component but also makes each piece more testable and maintainable. The functionality remains identical while clarifying the responsibilities of each part of the component.
| ); | ||
| } | ||
|
|
||
| return content; |
There was a problem hiding this comment.
issue (complexity): Consider extracting the submenu logic into a separate SubmenuWrapper component to simplify the MenuItem component and improve readability by reducing nested conditionals and isolating submenu-specific concerns
Consider extracting the submenu logic into its own component or hook. For example, you could create a dedicated `SubmenuWrapper` that handles the clone logic and open state. This would move the submenu processing out of `MenuItem`, reducing nested conditionals. For instance:
// New SubmenuWrapper.tsx
```tsx
import * as React from 'react';
import { mergeRefs, mergeProps } from '../../../hooks';
import { ListItemViewProps } from '../ListItemView/ListItemView';
import { MenuItemContext } from './MenuItemContext';
import type { MenuProps } from './types';
interface SubmenuWrapperProps {
submenu: React.ReactElement<MenuProps>;
content: React.ReactElement;
handleRef: React.Ref<any>;
onSubmenuOpenChange: (open: boolean) => void;
}
export function SubmenuWrapper({
submenu,
content,
handleRef,
onSubmenuOpenChange,
}: SubmenuWrapperProps) {
return (
<MenuItemContext.Provider value={{ setHasFocusInside: () => {} /* forward ref or handler as needed */ }}>
{React.cloneElement<MenuProps>(submenu, {
trigger: (triggerProps, triggerRef) =>
React.cloneElement<ListItemViewProps & { ref?: React.Ref<any> }>(content, {
ref: mergeRefs(triggerRef, handleRef),
componentProps: mergeProps(triggerProps, content.props.componentProps),
}),
onOpenChange: (open, event, reason) => {
submenu.props.onOpenChange?.(open, event, reason);
onSubmenuOpenChange(open);
},
})}
</MenuItemContext.Provider>
);
}Then update your MenuItem to use the new SubmenuWrapper:
if (submenu) {
return (
<SubmenuWrapper
submenu={submenu}
content={content}
handleRef={handleRef}
onSubmenuOpenChange={(open) => {
setSubmenuOpen(open);
if (!open) {
setHasFocusInside(false);
}
}}
/>
);
}This refactoring isolates submenu-specific concerns and reduces the nesting in MenuItem while keeping all functionality intact.
| const nodeId = useFloatingParentNodeId(); | ||
|
|
||
| React.useEffect(() => { | ||
| if (!tree) return; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!tree) return; | |
| if (!tree) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
| function handleTreeClick() { | ||
| // Closing only the root Menu so the closing animation runs once for all menus due to shared portal container | ||
| if (!parentId) { | ||
| onRequestClose(); | ||
| } | ||
| } |
There was a problem hiding this comment.
issue (code-quality): Avoid function declarations, favouring function assignment expressions, inside blocks. (avoid-function-declarations-in-blocks)
Explanation
Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers. Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you should use function expressions, which create functions in-scope.| function handleSubMenuOpen(event: {nodeId: string; parentId: string}) { | ||
| // Closing on sibling submenu open | ||
| if (event.nodeId !== nodeId && event.parentId === parentId) { | ||
| onRequestClose(); | ||
| } | ||
| } |
There was a problem hiding this comment.
issue (code-quality): Avoid function declarations, favouring function assignment expressions, inside blocks. (avoid-function-declarations-in-blocks)
Explanation
Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers. Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you should use function expressions, which create functions in-scope.
feat: support inline menu rendering feat(Popup): always render Popup in FloatingTree
It is a rework of current components,
MenuandDropdownMenu. More details in RFC.Summary by Sourcery
Implement a new Menu component with advanced features, including nested menus, keyboard navigation, and accessibility support
New Features:
Enhancements:
Tests: