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.

Sign up for GitHub

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

Jump to bottom

Minor change for handleMenuItemClick #582

Closed
arthur-clifford opened this issue Feb 26, 2021 · 8 comments · Fixed by #589
Closed

Minor change for handleMenuItemClick #582

arthur-clifford opened this issue Feb 26, 2021 · 8 comments · Fixed by #589

Comments

@arthur-clifford
Copy link
Contributor

There is a simple change which can be made to allow for more dynamic decisions as to whether to hide the header menu for a given command.

In plugins/headerMenu.js

For the handleMenuItemClick function:

hideMenu() is called prior to checking whether there is a command.

This means every time you click a menu option the menu closes.

If you move hideMenu after the if block and wrap it as follows:

if(!e.isDefaultPrevented) {
  hideMenu();
}

The result is the menu will only be hidden if a developer doesn't use e.preventDefault() in the onCommand handler.

onCommand: (e: Event, args: MenuCommandItemCallbackArgs) => {
   e.preventDefault();
  // do stuff that will not close the window.
}

This would also mean a developer's could decide whether to prevent the default depending on the command that is being handled.
@6pac
Copy link
Owner

6pac commented Mar 1, 2021

OK, have done this. it will be in the next release. @ghiscoding take note, it seemed trivial and a good idea.

@6pac 6pac closed this as completed Mar 1, 2021
@ghiscoding
Copy link
Collaborator

ghiscoding commented Mar 1, 2021 •
edited
Loading

@6pac go ahead for the changes, please note that it's in 5 different plugins

  • grid menu
  • header menu
  • header button
  • cell menu
  • context menu

@6pac
Copy link
Owner

6pac commented Mar 1, 2021

OK, it appears that the code change is just in HeaderMenu.js, the others call that, so it's a flow on effect. It is non-breaking, though.

@ghiscoding
Copy link
Collaborator

ghiscoding commented Mar 1, 2021 •
edited
Loading

if I look at the Grid Menu, it seems that I added a leaveOpen option instead of the prevent default cancellation. It's probably better to add the same prevent default code to the Grid Menu as well, to be more consistent.

SlickGrid/controls/slick.gridmenu.js

Lines 507 to 510 in 3991679

var leaveOpen = (_options.gridMenu && _options.gridMenu.leaveOpen) ? true : false;
if (!leaveOpen) {
hideMenu(e);
}

@6pac
Copy link
Owner

6pac commented Mar 1, 2021

10-4

@ghiscoding
Copy link
Collaborator

@6pac did you do code change for that or did you wanted me to create a PR?
Just want to make sure it's not forgotten

Cheers mate

@6pac
Copy link
Owner

6pac commented Mar 16, 2021

thanks @ghiscoding maybe better if you do it...

@ghiscoding ghiscoding reopened this Mar 16, 2021
ghiscoding added a commit that referenced this issue Mar 17, 2021
@ghiscoding
fix: add preventDefault to optionally leave plugin menu open
a7ffc4e 
- fixes #582
- the original issue only mentioned the HeaderMenu plugin but I went ahead and applied the same logic (and tested them all) on the following plugins
   - HeaderMenu
   - GridMenu
   - ContextMenu
   - CellMenu
@ghiscoding ghiscoding mentioned this issue Mar 17, 2021
Merged
@ghiscoding
Copy link
Collaborator

ghiscoding commented Mar 17, 2021 •
edited
Loading

done... and I went further and applied it to all plugins with menu that I found (4 of them) to have such behaviors.

@6pac 6pac closed this as completed in #589 Mar 21, 2021
6pac pushed a commit that referenced this issue Mar 21, 2021
@ghiscoding
fix: add preventDefault to optionally leave plugin menu open (#589)
c7dfe45 
- fixes #582
- the original issue only mentioned the HeaderMenu plugin but I went ahead and applied the same logic (and tested them all) on the following plugins
   - HeaderMenu
   - GridMenu
   - ContextMenu
   - CellMenu
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: add preventDefault to optionally leave plugin menu open 6pac/SlickGrid
3 participants
@6pac @ghiscoding @arthur-clifford