PerfectMemory / ngx-contextmenu

A context menu component for Angular
https://perfectmemory.github.io/ngx-contextmenu
MIT License
42 stars 12 forks source link

closeAllContextMenus not possible with ngx-contextmenu after Version 7 #9

Closed timonkrebs closed 2 years ago

timonkrebs commented 2 years ago

Describe the bug We have multiple contextmenus on our page. There are some actions that should trigger the contextmenu to close. We can not know which contextmenu is open at any time. Therefore we need a way to make sure all contextmenus get closed.

The main problem is that closeAllContextMenus and getLastAttachedOverlay are not in the API after Version 7.0.0

sroucheray commented 2 years ago

Hi @timonkrebs,

Good point. As I already said in another issue https://github.com/PerfectMemory/ngx-contextmenu/issues/8#issuecomment-1173741278 I cleaned up the API between 7 and 8 because there were too many exposed public APIs. It was a bit hard to maintain. In particular I thought that using the service was an advanced case.

I think your use case might be valid. But before digging into some implementation I need to know more. Could you tell me more about those "actions" you mentioned that should close the submenus ? What are they ? Would it be of interest to take them into account directly in the lib ?

timonkrebs commented 2 years ago

The main problem is that we have custom shortcuts (eg. Shift+Y ...) where we can change modes of the page. When the mode changes we need to close all existing contextmenus. But there are also other usecases where we would like to be able to ensure that all contextmenus get closed.

sroucheray commented 2 years ago

Thanks @timonkrebs ! I now better understand your reason. IMHO while it is a very legitimate case, it is a edge case. I am hesitant to reintroduce the global service because it adds another primitive to this simple library and increases its complexity. What I could do is just add a close method to the ContextMenuDirective. Then you would be able to call it on all instances of the directive on any event (via a custom service of yours for example).

With the suggestion I did on the other issue https://github.com/PerfectMemory/ngx-contextmenu/issues/8#issuecomment-1214179437, you even could do something like:

<div
  #ngxContextMenu="ngxContextMenu"
  (window:keydown.shift.Y)="ngxContextMenu.close()"
></div>

What do you think ?

timonkrebs commented 2 years ago

I see your point and i think this should work. But then we would need to track which contextmenus are open and close them manually if there are open ones. To do that we would need to store the instances of the open contextmenus. This feels like internal knowledge of the lib. Don't you think?

timonkrebs commented 2 years ago

Or would i be better to have the functionality to get all open contextmenues from the service. Then closing them could be done by the consumer of the lib.

sroucheray commented 2 years ago

Actually you could call close on all contexmenus whatever their state. No need to keep track of open menus. When calling close, if the contextmenu is open then it closes obviously but if it is already closed then nothing would happen.

timonkrebs commented 2 years ago

Good to know. But we have a lot of contextmenus and they are not all available at the same time. There are contextmenus for the different modes etc. We would still need to have all the contextmenus in one list. And thats not that easy, since we do load some components lazily. Therefore we do not have all the contextmenues that could be open in the future.

sroucheray commented 2 years ago

Can you elaborate on why you would need to maintain a list of contextmenus ? As I understand it, you would need it to close them all but with my suggestion not any more. You would only need a service with a subject emitting a close event. You would inject the service wherever you instantiate a contextmenu and call close on it when the service emits. Then you can bind your change mode or any other event to emitting a close event on the service.

timonkrebs commented 2 years ago

You are right. That would work. I think i did not think of that because it also feels like unnecessary boilerplate. But on the other hand this would give much more control to the consumer of the contextmenu. I am also not sure about performance if there are a lot (>10'000) of contextmenus on the page?

sroucheray commented 2 years ago

10 000 is not that a big number and usually the DOM is slow but we won't touch it if menus are closed. I would not be to much concerned about performance before implementation.

Question would you like to implement it and make a pull request ? If no, no problem. If yes I will write a separate issue for the specs and I'll guide you.

sroucheray commented 2 years ago

Wrote the specifications for the implementation https://github.com/PerfectMemory/ngx-contextmenu/issues/10

timonkrebs commented 2 years ago

I will do it if it is not already done until i find the time for it.

sroucheray commented 2 years ago

I implemented open and close methods on the ContextMenuDirective. I consider that it closes this issue. If you think this is not the case, please open another issue.