Add Keyboard navigation on IconPicker #2778
Conversation
Co-authored-by: Matheus <[email protected]>
|
@gitstart-twenty Please use SelectableList component and its API to implement it :) |
Alright |
gitstart-twenty
changed the title
Co-authored-by: v1b3m <[email protected]> Co-authored-by: Matheus <[email protected]>
Co-authored-by: v1b3m <[email protected]> Co-authored-by: Matheus <[email protected]>
Co-authored-by: v1b3m <[email protected]> Co-authored-by: Matheus <[email protected]>
gitstart-twenty
added
-PR: awaiting review
and removed
-PR: awaiting author
-PR: wip
labels
There was a problem hiding this comment.
@gitstart-twenty I have left comment about performance. Other than that, looks good to me
|
|
||
| const setSelectableItemIds = useSetRecoilState(selectableItemIdsState); | ||
| const isSelectedItemId = useRecoilValue(isSelectedItemIdSelector); | ||
| const selectedItemId = useRecoilValue(selectedItemIdState); |
There was a problem hiding this comment.
@gitstart-twenty We cannot expose selectedItemId as part of useSelectableList() By doing that, you are listening to any change of selectedItemIdState.
So any component importing useSelectableList will re-render which will considerably impact performances for high level components
| @@ -98,6 +100,11 @@ export const CommandMenu = () => { | |||
| setSearch(event.target.value); | |||
| }; | |||
|
|
|||
| const { selectedItemId } = useSelectableList({ | |||
There was a problem hiding this comment.
we can't do that (see my comment below)
Instead, within each command menu command we should use isSelectedItemId = useSelectableList(itemId).
This will only listen to the state tied to the itemId
| (cmd) => cmd.id === selectedItemId, | ||
| ); | ||
|
|
||
| selectedCommand?.onCommandClick?.(); |
There was a problem hiding this comment.
so this logic should be moved into CommandMenuItem
| @@ -77,49 +82,81 @@ export const IconPicker = ({ | |||
| ).slice(0, 25); | |||
| }, [icons, searchString, selectedIconKey]); | |||
|
|
|||
| const iconKeys2d = useMemo( | |||
| () => arrayToChunks(iconKeys.slice(), 5), | |||
Co-authored-by: v1b3m <[email protected]> Co-authored-by: Matheus <[email protected]>
|
@charlesBochet We're on this |
Co-authored-by: v1b3m <[email protected]> Co-authored-by: Matheus <[email protected]>
| }, [navigate, onClick, to, toggleCommandMenu]); | ||
|
|
||
| useEffect(() => { | ||
| if (enterClicked && isSelectedItemId) { | ||
| onItemClick(); | ||
| resetEnterClicked?.(); | ||
| } | ||
| }, [enterClicked, isSelectedItemId, onItemClick, resetEnterClicked]); |
There was a problem hiding this comment.
Hey @charlesBochet I had some issues implementing this solely from the CommandMenuItem, what do you think of this approach?
|
@gitstart-twenty I have implemented it on IconPicker fully and partially on CommandMenu. On CommandMenu, we should leverage the commandMenuCommandsState better. Our onEnter callback should filter commandMenuCommands based on itemId. Once the right command found, we can trigger onItemClick(onClick, to) This will work with the static commands but not with the records (companies, people, activities). I think these should be pushed to commandMenuCommands too and then it will work too |
|
@gitstart-twenty I'm about to rework the frontend folder architecture. I'm merging this PR as it's adding a feature on IconPicker. |
Fixes #2746