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

Asynchronous create sub menu's after context menu has opened. #429

Closed
Ruud-cb opened this issue Jul 19, 2016 · 17 comments
Closed

Asynchronous create sub menu's after context menu has opened. #429

Ruud-cb opened this issue Jul 19, 2016 · 17 comments
Labels

Comments

@Ruud-cb
Copy link

Ruud-cb commented Jul 19, 2016

Hi,

I've been searching the past 2 days for a perfect context menu, this one and another one stand out above all others. To put it in perspective: I am developing an app with AngularJS. Thus I rather have a angular context menu then a default jquery menu.
One came really close: https://github.com/Templarian/ui.bootstrap.contextMenu
Yet this one was not so nice with providing icons, keyboard events some other things. But it does has one major improvement above this one: Async create sub menu's. Instead of providing an item I can provide an promise, which would be loaded once the user has opened the sub-menu.

Perhaps it would be possible to implement something similar to this? So instead of only providing items, or a list of items, provide a promise, and once it is completed it will render the sub menu's.

I know the documentation provides something with async loading the context menu, but that's not what I am looking for, that happens before everything is created. This should happen once the context menu has already been created, either after the menu has opened, or once the user wants to open the sub menu.

See the plunker for an example using that angular directive: https://embed.plnkr.co/Bebmjn2aAlflxYe1cwIS/

Similar to #421, but then new created sub menu's.

@bbrala
Copy link
Member

bbrala commented Jul 19, 2016

I understand that to get this to work nicely with Angular you would like to use promises like that. To be honest, I only have a small bit of experience with Angular. I can't really predict how much effort it would take to implement.

I wouldn't be against a way to do this, but i wonder if it is even possible to do it properly without rewriting tons of code.

Anyways I won't be implementing this, but if someone with more experience wants to, it would be fine to talk about how we would do that.

@bbrala bbrala added the Feature label Jul 19, 2016
@Ruud-cb
Copy link
Author

Ruud-cb commented Jul 20, 2016

Hi @bbrala, Thanks for responding. Angular indeed covers this nicely, but is similar to jquery promises which seems to do the same.
Thanks for marking it as a Feature, we need it any way so I might be implementing it. Of course, this would then need to be done without Angular, by using the Jquery promises.
Indeed it would be a tough task to accomplish, I will first have to take a good look at the code, with some POC I will check if it is possible and then I will let you know for a possible pull request.

@bbrala
Copy link
Member

bbrala commented Jul 20, 2016

Would be quite awesome if you could find the time to do that! If you have any questions regarding the library, just put it here or send me an email. :)

@Ruud-cb
Copy link
Author

Ruud-cb commented Jul 25, 2016

Hi @bbrala, I submitted a pull request. There are still some todo's left, which I wonder for what the easiest way is to check if the context menu is open or not (to fix todo no. 2). The other sub-sub-menu promises problem: Not sure where the problem occurs, it does everything correctly on the first time, but once it has been initialized for the first time, it breaks on the second time. See this plunker: https://embed.plnkr.co/6cZo2hCy1a4FRK8GqeT6/

I think best is to throw an error if the user wants to do sub-sub-menu promises?

@bbrala
Copy link
Member

bbrala commented Jul 25, 2016

Thanks again for the work.

I feel like the problemens that arrise on the second open are a scoping issue or perhaps realtive to what menu the promise is called. Every submenu is pretty much a new instance, but then opened with its parent as root. This might be related to your problem.

I will have a look later this week to see if I see any glaring problem with the code. And we do need to make sure there are unit tests for this functionality, if you dont feel confortable making those in the qunit framework in place i could have a look.

Anyways, i'll start with having a good look later this week.

@Ruud-cb
Copy link
Author

Ruud-cb commented Jul 25, 2016

I fixed the issue regarding the sub-sub-menu promise. I noticed that it got a class context-menu-input which is applied on this line. Would be correct normally, but we have assigned the type 'sub' and call op.create again. Doing an extra check solved this issue.
In my plunker, the problem could be avoided by really creating a new subItems variable instead of one that is globally defined. Because the promise was already resolved for items : loadItems2().

Only 2 things left are avoiding the console error when the context menu is closed while the promise is resolved later and creating unit tests. I agree on creating unit tests, hope you can find the time to do this.

I'll update the pull request.

@bbrala
Copy link
Member

bbrala commented Aug 26, 2016

Hi again, pretty awesome :) I added some comments in the PR.

Some todo's to get this up and running:

  • Check if a menu is open when creating the items when a promise is done.
  • Create a demo in the documentation
  • Create documentation on this feature.
  • Create tests for this
  • Refactor the handling of item.type === 'sub', mostly placement, but could do with a private function.

@Ruud-cb
Copy link
Author

Ruud-cb commented Aug 26, 2016

Hi, yes I've updated the code for the refactor and accepting string for errors. The demo is still available on my plunker which you could use to update the documentation.
Not sure if you are asking me to finish this list? Before you suggested to do make the tests.

@bbrala
Copy link
Member

bbrala commented Aug 26, 2016

Thanks! No not necessarily, just need to remember to do those things before merging. :)

@bbrala
Copy link
Member

bbrala commented Oct 24, 2016

Well, got it working as it should. Loading icon, proper handle if menu is closed already, and a demo. Couldn't get QUnit to play nice unfortunately. :(

@bbrala
Copy link
Member

bbrala commented Oct 24, 2016

Merged the changes. Thanks again @Ruud-cb

@bbrala bbrala closed this as completed Oct 24, 2016
@Ruud-cb
Copy link
Author

Ruud-cb commented Oct 25, 2016

Nice! Thanks for merging!

@Ruud-cb
Copy link
Author

Ruud-cb commented Oct 25, 2016

Hmm.. I only have one issue left now:
image

classNames is undefined, where opt is in this scope a menu item. It does have it's own custom loading icon implementation by providing an icon function, but also without that custom function I'm not able to avoid this error.

Not sure what or how the loading indicator is doing/showing but should be optional?

bbrala added a commit that referenced this issue Oct 25, 2016
@bbrala
Copy link
Member

bbrala commented Oct 25, 2016

I had a bad commit day yesterday... wow. Fixed it.

@Ruud-cb
Copy link
Author

Ruud-cb commented Oct 26, 2016

No problem, no issues now, just that I don't like it that way and still disabled it 😄

@bbrala
Copy link
Member

bbrala commented Oct 26, 2016

Hehe fair enough. :)

@Ruud-cb
Copy link
Author

Ruud-cb commented Oct 28, 2016

Perhaps it is possible to set a boolean to true or false to show and hide that loader.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants