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

New contextMenuBuilder api and several additions #27

Closed
wants to merge 5 commits into from

Conversation

SuricateCan
Copy link
Contributor

Several additions:

@Templarian After this PR is accepted (if you choose to) I will work on having the context menu built from a template inside the ngRepeat context.

Several additions:
* Issues:
   - Templarian#22 Adding a context menu class name to dropdown: Class ng-bootstrap-contextmenu is now appended to the holding div;
* Pull Requests
   - Templarian#17 Added a event for opening: Added event for opening, after open and after closed;
   - Templarian#21 Allow icons (as HTML) in menu item: Not as html, but the new builder api handles the icon submission;
   - Templarian#25 Added support for item text promise: The text is now wrapped inside a $q.when function.
@Templarian
Copy link
Owner

That's a lot of breaking changes. I can't really merge that right now. You have my permission to create a new context menu from this project though.

I'll look into the code changes eventually and try to implement them in without breaking the legacy code. I use this widget in tons of client projects and personal projects (as do many others). I have to ensure some consistency in how it works.

@Templarian Templarian closed this Oct 13, 2015
@SuricateCan
Copy link
Contributor Author

@Templarian You dont have to worry about breaking old codes. I kept compatibility with the older versions.
My own project works under the old code standards, and the new one did not break it, I made sure of it.
I appreciate you giving me permission to create a new project using this one, but I rather just contribute to yours.

@Templarian Templarian reopened this Oct 13, 2015
@Templarian
Copy link
Owner

Oh okay, will have to look into it some more then. I only skimmed the changes you made to the readme.

@SuricateCan
Copy link
Contributor Author

I must have forgotten to mention backwards compatibility.
I thought I added that line...

@josetaira
Copy link
Collaborator

Seems this PR has become conflicting. I've added in a new PR for the class name issue at #60

I'll look into the other PRs and see how to merge them into the most recent master. If you have enough time, @SuricateCan, would you be able to redo some of these?

@SuricateCan
Copy link
Contributor Author

Hi @josetaira , I tried to merge upstream but there has been many changes that virtually break this PR, so I gave up on it. I would have to re-write it entirely from the current master, and I just don't have the time right now...

@josetaira
Copy link
Collaborator

Yeah, refactoring does that to pending PRs. I'll try to work on each feature when I can, and post progress under different PRs so that efforts don't get duplicated. I'll close this for now, maybe we can just reference it in the future, since it's quite impossible to merge it cleanly without too much work.

@josetaira josetaira closed this Apr 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants