Skip to content

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

Merged
merged 7 commits into from
Feb 4, 2022
Merged

Conversation

sideDrawerRef,
updateCell,
} = useProjectContext();
const { userDoc, userClaims } = useAppContext();
const { setContextMenu } = useContextMenuAtom();
Copy link
Contributor

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

Comment on lines +11 to +15
export interface IContextMenuActions {
label: string;
icon: React.ReactNode;
onClick: () => void;
}
Copy link
Contributor

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.

const handleContextMenu = (
e: React.MouseEvent<HTMLDivElement, MouseEvent>
) => {
e.preventDefault();
if (contextMenuRef?.current) contextMenuRef?.current?.setAnchorEl(e.target);
setAnchorEle?.(e?.target as HTMLElement);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
setAnchorEle?.(e?.target as HTMLElement);
setAnchorEle(e?.target as HTMLElement);

I thought setAnchorEle would always exist/never be undefined

Copy link
Contributor Author

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

Comment on lines +14 to +40
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,
};
}
Copy link
Contributor

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

Copy link
Contributor Author

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

@notsidney notsidney merged commit d7cc479 into rowyio:develop Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants