-
Notifications
You must be signed in to change notification settings - Fork 15
fix(container-menu): ensure outside clicks always close dropdown menu #699
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
Conversation
| } | ||
|
|
||
| return () => { | ||
| clearTimeout(timeoutId); |
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.
Memory leaks are unlikely here since setTimeout(..., 0) runs almost immediately. However, it's still good to follow best practices.
| // Reset `focusOnOpen` if it was previously set to true; this prevents | ||
| // forced focus on the initial item in future state updates. | ||
| if (state.focusOnOpen) { | ||
| changes = { ...state, focusOnOpen: 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.
The useEffect hook resets focusOnOpen on update, eliminating the need for an explicit dispatch call. This method has proven to be more reliable, especially with nested submenus.
|
|
||
| export const StateChangeTypes: Record<string, string> = { | ||
| FnSetStateRefs: 'fn:setStateRefs', | ||
| FnInternalUpdate: 'fn:internalUpdate', |
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.
Although the new naming might be preferable, are we concerned this introduces a corner case breaking change? Would it make sense to leave 'fn:setStateRefs' as the change type name?
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.
This is the only stateChangeType that wasn’t previously exposed to consumers through the onChange callback, so it should be safe to rename without causing any issues.
Description
Fixes an issue where clicking outside a menu dropdown does not close it if a nested item is auto-focused.
Details
Checklist
npm start)