Skip to content
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

Feat: onRenderContextMenu return false #399

Closed
wants to merge 2 commits into from

Conversation

1pone
Copy link
Contributor

@1pone 1pone commented Jan 30, 2024

Always hide the context menu when onRenderContextMenu return false

@1pone
Copy link
Contributor Author

1pone commented Jan 30, 2024

Optimize for this pr #396

@josdejong
Copy link
Owner

Thanks! I think the approach to extend onRenderContextMenu to return false is the right approach. However I think implementing it neatly requires a bit more refactoring internally. The onRenderContextMenu callback should be invoked only once, and we should not introduce a "test call" with empty items to see if it does return false (that can lead to unexpected behavior.

Right now, onRenderContextMenu is called inside TreeContextMenu and TableContextMenu. I think this call should be pulled out of those components, it should be moved to the two openContextMenu() implementations: invoke onRenderContextMenu, if it returns false, cancel opening the modal, and otherwise, pass the items to the modal. Does that make sense?

@1pone
Copy link
Contributor Author

1pone commented Mar 4, 2024

Thanks for the suggestion, I've tried to rewrite this part of the code in #411, can you help me see if there's any problem?

josdejong pushed a commit that referenced this pull request Mar 8, 2024
BREAKING CHANGE:

The `onRenderContextMenu` callback now also triggers when the editor is `readOnly`,
so you now have to handle that case in the callback.
@1pone 1pone closed this Mar 8, 2024
@josdejong
Copy link
Owner

Published now in v0.23.0

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.

3 participants