Refactor MXContextMenu #1626
Refactor MXContextMenu #1626
Conversation
0dc8719 to
a23b666
Compare
| if (ctxMenu) { | ||
| ctxMenu.classList.add('generic-context-menu-close'); | ||
| useEffect(() => { | ||
| function hideContextMenu() { |
There was a problem hiding this comment.
Might be worth investigating the use of Bootstrap's Overlay, which has a rootClose prop to deal with hiding on click anywhere else. Anyway, not for this PR, it's more FYI in the broader scope of merging together the various context menu implementations present in the codebase.
There was a problem hiding this comment.
Oh interesting didn't know about that, I will definitely check that out thanks!
|
|
||
| export default function MXContextMenu(props) { | ||
| const { children } = props; | ||
| const contextMenu = useSelector( |
There was a problem hiding this comment.
I would destructure this, maybe on a separate line, and then list the specific variables needed in the useEffect dependencies array.
There was a problem hiding this comment.
Makes sense, I will update:)
a23b666 to
39789da
Compare
39789da to
ac70e04
Compare
|
|
||
| let posxoffset = 10; | ||
| if (menuEndPos > windowWidth) { | ||
| posxoffset = contextMenu.offsetWidth; |
There was a problem hiding this comment.
@walesch-yan sorry a bit late
i think this PR introduce a regression
the context menu lost it's click position when the page is scroll
There was a problem hiding this comment.
You are right. The idea was to replace the menu inside the viewable window (in y-direction) to prevent scrolling, but it is missing the value of the offset from the top of the page (I.e. when scrolling down a little, it will always pop up on top as we are below the the windowheight offset).
There was a problem hiding this comment.
I will make a new PR to fix this
There was a problem hiding this comment.
Thanks for pointing it out!:)
This PR refactors the MXContextMenu into a functional component and replaces the
.cssfile with a module.2 minor changes: