2727-fix(front): CommandMenu and KeyboardMenu invoke handled #2783
Conversation
| @@ -89,14 +91,20 @@ export const CommandMenu = () => { | |||
| const isCommandMenuOpened = useRecoilValue(isCommandMenuOpenedState); | |||
| const [search, setSearch] = useState(''); | |||
| const commandMenuCommands = useRecoilValue(commandMenuCommandsState); | |||
|
|
|||
| const { closeKeyboardShortcutMenu } = useKeyboardShortcutMenu(); | |||
| const isKeyboardShortcutMenuOpened = useRecoilValue( | |||
There was a problem hiding this comment.
@Kanav-Arora We don't want to use the states directly outside of its component. This allow us to provide a nice API for our components.
This is also to optimize for re-renders. Here it's not too bad but, every time isKeyboardShortcutMenuOpened changed, the whole CommandMenu will re-render. That's why, while designing Keyboard shortcut menu API (aka useKeyboardShortcutMenu), we really need to ask ourselve what we want to expose in order to not push our component users to not trigger re-renders.
In this particular case, I think we don't ant to listen at this isKeyboardShortcutMenuOpenedState at all, we want to close the keyboardShortCutMenu in any case, worst case nothing happens
| @@ -20,9 +22,14 @@ export const KeyboardShortcutMenu = () => { | |||
| const isKeyboardShortcutMenuOpened = useRecoilValue( | |||
| isKeyboardShortcutMenuOpenedState, | |||
| ); | |||
| const isCommandMenuOpened = useRecoilValue(isCommandMenuOpenedState); | |||
There was a problem hiding this comment.
@Kanav-Arora I'm merging it with changes (see comments) because we are planning to move many files over the week-end, it would be too bad that your PR would have many conflicts.
I'm also including an unrelated fix that I found while working on your PR
Thank you!
Closes #2727