-
Notifications
You must be signed in to change notification settings - Fork 537
Right click menu -v2 #632
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
Right click menu -v2 #632
Conversation
9b04a0f
to
b46d694
Compare
src/components/Table/index.tsx
Outdated
sideDrawerRef, | ||
updateCell, | ||
} = useProjectContext(); | ||
const { userDoc, userClaims } = useAppContext(); | ||
const { setContextMenu } = useContextMenuAtom(); |
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.
Can you check if calling this causes unnecessary re-renders?
This calls useAtom(contextMenuAtom)
, which subscribes this component to that atom’s state, so it might cause Table
to re-render when the context menu is opened.
We might have to use useUpdateAtom
here. The CodeSandbox on this page illustrates what I’m talking about: https://jotai.org/docs/utils/use-update-atom
export interface IContextMenuActions { | ||
label: string; | ||
icon: React.ReactNode; | ||
onClick: () => void; | ||
} |
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.
Note for the future: when we have more context menu actions for different field types, we should move this to src/components/fields/types.ts
. I’m also not sure about having this file be inside _BasicCell
. This is fine for now however.
src/components/fields/_BasicCell/BasicCellContextMenuActions.tsx
Outdated
Show resolved
Hide resolved
b46d694
to
57245aa
Compare
57245aa
to
afdd98f
Compare
64754ec
to
5736636
Compare
const handleContextMenu = ( | ||
e: React.MouseEvent<HTMLDivElement, MouseEvent> | ||
) => { | ||
e.preventDefault(); | ||
if (contextMenuRef?.current) contextMenuRef?.current?.setAnchorEl(e.target); | ||
setAnchorEle?.(e?.target as HTMLElement); |
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.
setAnchorEle?.(e?.target as HTMLElement); | |
setAnchorEle(e?.target as HTMLElement); |
I thought setAnchorEle
would always exist/never be undefined
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.
You are right. Ill change it
export function useSetAnchorEle() { | ||
const setAnchorEle = useUpdateAtom(anchorEleAtom); | ||
return { setAnchorEle }; | ||
} | ||
|
||
export function useSetSelectedCell() { | ||
const setSelectedCell = useUpdateAtom(selectedCellAtom); | ||
return { setSelectedCell }; | ||
} | ||
|
||
export function useContextMenuAtom() { | ||
const [anchorEle] = useAtom(anchorEleAtom); | ||
const [selectedCell] = useAtom(selectedCellAtom); | ||
const resetAnchorEle = useResetAtom(anchorEleAtom); | ||
const resetSelectedCell = useResetAtom(selectedCellAtom); | ||
|
||
const resetContextMenu = async () => { | ||
await resetAnchorEle(); | ||
await resetSelectedCell(); | ||
}; | ||
|
||
return { | ||
anchorEle, | ||
selectedCell, | ||
resetContextMenu, | ||
}; | ||
} |
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.
I’m not sure of the benefits of creating custom hooks here, instead of accessing the atoms directly using useUpdateAtom
in Table
for example
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.
I don't recall where I saw. But I pick up this pattern a while back because it's used to hide the imports
//no custom hook
import { anchorEleAtom } from '@src/store/contextmenu'
import { useAtom } from 'jotai'
const [anchorEle] = useAtom(anchorEleAtom)
//custom hooks
import { useContextMenu } from '@src/store/contextmenu'
const { anchorEle } = useContextMenu()
it's purely syntactic sugar I think. We can remove it if necessary
Update conextMenuAction's type definition
Add snackbar
Convert to Jotai
Add optional shortcut label
Set MenuList to dense
Rename MenuRow Component to ContextMenuItem
Covert to async/await style