Templarian / ui.bootstrap.contextMenu

AngularJS Bootstrap UI Context Menu
MIT License
259 stars 127 forks source link

3 Improvements #12

Closed SuricateCan closed 9 years ago

SuricateCan commented 9 years ago

Hi, I did 3 improvements here.

  1. Adjusted the directive declaration to function with strict-di set;
  2. Added a third function to the menuOptions array to disable items on the fly. If no function is present, the item is enabled by default;
  3. Added a tag attribute to help people (like me) that use this context menu outside a ngRepeat context.

For people updating the plugin, there will be NO necessary changes to old code.

Hope you enjoy!

SuricateCan commented 9 years ago

The second commit is for simplicity sake of item 2 above. Instead of a function to DISable the menu item, a function to ENable. This simple stuff can prevent a lot of headaches...

Templarian commented 9 years ago
  1. Good catch, oversight on my part.
  2. Awesome!
  3. Uhh... not sure this one is needed. If you are not in an ng-repeat, couldn't you just access it within the $scope variable of the controller like normal. It should be in scope. Unless I'm misunderstanding this.
SuricateCan commented 9 years ago

Not really.

Let me show you my scenario: I have 2 menus (collections) in my page.

One is loaded dynamically in angular (does not need the tag) and have its isolated scope. The other is loaded on the server side and is written directly into the template (no ngRepeat).

In my application, they both have the same key to the item (an ID property). So I added the tag to both contexts and use the same item collection.

SuricateCan commented 9 years ago

As the second list is written in the template, all contexts have the same scope, making impossible to identify the item. The alternative would be to use menuOptions(item) instead, but then I find the tag option more explicit and elegant (and reusable).

Templarian commented 9 years ago

Sorry forgot to reply. I'm accepting this pull request, just going to be renaming contextMenuTag to model. Other than it will probably go in as is. Busy at work so will merge the pull request Tuesday and update the bower package.

Thanks again for writing this pull request, just wanted to give an update.

SuricateCan commented 9 years ago

Sure, no problem. I just dont think model is a good name for it. We simple dont handle it as a model (binds and all). But its up to you.

kasoban commented 9 years ago

@SuricateCan : Thanks, the feature for disabled entry was just what I was looking for. I did some local modifications though: In the disabled case you apply no click handler, thus the default click handlers trigger when clicking on the disabled menu option, which was undesirable in my case.

Thus I added the click handler with preventDefault for the disabled case. Maybe you can amend that to your PR? I don't see how I can modify it.

kasoban commented 9 years ago

Seems like creating a PR for your fork is/was the way to go? I'm still not sure :smile: Anyway, here is what I tried to describe: https://github.com/SuricateCan/ui.bootstrap.contextMenu/pull/1

Templarian commented 9 years ago

A pull request for a pull request. :laughing: I'll be putting this in tonight after some testing.

Thanks @kasoban!

SuricateCan commented 9 years ago

Hey @Templarian I merged @kasoban changes. You are good to go! :D

Templarian commented 9 years ago

Thanks @SuricateCan!

//edit, wrong name

Templarian commented 9 years ago

Everything is merged and synced up. The latest version in bower is now 0.9.3.

Thanks again! These additions should be very useful for others.

Templarian commented 9 years ago

Also, @SuricateCan I updated the demo with an example of the model for those not using ngRepeat. The demo is now on codepen.io.

SuricateCan commented 9 years ago

Very nice! Thanks!