implement #27 - adds an api for more direct usage #29
Conversation
src/ctxmenu.ts
Outdated
| } | ||
| } | ||
|
|
||
| private openMenu(ctxMenu: CTXMenu, e: MouseEvent) { |
There was a problem hiding this comment.
I would move the content of openMenu back to the show function if possible, to avoid confusion (as discussed before)
src/ctxmenu.ts
Outdated
|
|
||
| const newMenu = beforeRender([...ctxMenu], e); | ||
| this.menu = this.generateDOM(newMenu, e); | ||
| document.body.appendChild(this.menu); | ||
| this.openMenu(newMenu, e); |
There was a problem hiding this comment.
I'd try replacing this with the show function
src/ctxmenu.ts
Outdated
| @@ -196,18 +205,33 @@ class ContextMenu implements CTXMenuSingleton { | |||
| t.removeEventListener("contextmenu", o.handler); | |||
| delete this.cache[target]; | |||
| } | |||
|
|
|||
| public show(ctxMenu: CTXMenu, e: MouseEvent) { | |||
There was a problem hiding this comment.
The only thing I don't like about the current implementation of your PR is the necessity to provide this MouseEvent in a public API. This forces the caller to carry around an event. In case of a button press it might be simple enough, but with some complex logic I can imagine this getting cumbersome very quickly. Also the user of this function might want to open a menu in other cases where there's no MouseEvent, too (key presses for example)...
Since subsequently generateDOM will be called, which itself supports an HTMLElement instead of an event, we might want to provide this possibility here, too by overloading the show function.
Providing just the target coordinates or a DOMRect might be handy, too, although this might require some further changes.
In case you need to change the type of the second parameter for generateDOM from HTMLLIElement to HTMLElement, feel free to do so, it doesn't seem to cause any issues.
There was a problem hiding this comment.
Let me know your thoughts on this! 😊
There was a problem hiding this comment.
I think adding an overload to the show method using a HTMLElement instead of a MouseEvent is a good idea, especially since it does not require any real effort :)
Directly passing coordinates also sounds interesting, but I would assume that you would usually always have a reference to an event or Element. So I think I would leave that extension for a future pull request ;)
There was a problem hiding this comment.
I did change it as described. However I switched to using just a union type on the parameter instead of using overloads. I am not sure if your were using overloads for any particular reason, as far as I can tell, using a union type for the parameter should be enough - let me know if you want to change this back if this was intended for some special reason.
There was a problem hiding this comment.
I'm not so sure, why, either 🤣
I think what I like most about overloads over union types is that the type hints are concise, since it only shows what you are using instead of all possibilities at once.
No need to change it though. ( I'll do it myself later, if it bothers me...)
I'm having a look right now and if all looks fine, I'll merge the PR!
|
I agree that the |
- always use show method instead of private openMenu method
- show method should also work with a HTMLElement instead of a MouseEvent
As promised, a pull request to add apis for controlling a context menus lifecycle directly.
I think I got to all the points you mentioned in #27.
One note on the implementation:
As suggested, I merged the private
closeMenumethod with the new publichidefunction. I choose not to expose the optionalmenuparameter in the public interface. As far as I can tell, this is only really used when closing a submenu, so I don't think we need in the public api.Let me know what you think.