Propose refactor for dropdown toggle.

[bekos]

• Restict on EA (dropdown-toggle)
• Use template to render bootstrap's html
  (open is not hardcoded in the code).
• Add option for dropdown to be initially open.
• Add tests & demo examples.

[pkozlowski-opensource]

@bekos Thnx foropening this PR, was thinking about this change for quite some time but didn't have time / idea on how to tackle it properly. I'm adding some comments to the code to kick-off the discussion.
On a separate note we should make sure that this PR addresses #270 opened today.

@ajoslin would be totally awesome if you could help us on this one.

@bekos BTW, added you to the team, you have now the full commit access. Welcome!

[bekos]

@pkozlowski-opensource Thank you for the welcome :-)

About the pull-right option, maybe we can add a placement attribute like the one on your proposal for tabs orientation. ng-class='placement && "pull-" + placement' inside dropdown-menu element. There is no bottom orientation this time!

I will add test for #270 and probably change my approach about click events. I'll look into it tomorrow.

About the <div> or <li> element as root in the template, i have no strong opinion about how we can solve this. An option is to define the element in the compile function or use two different directives dropdown-list-menu and dropdown-menu.

[bekos]

@pkozlowski-opensource Can you please take a look on this one? Finally i used the same controller for different directives (different template). This way i could implement all kind of dropdowns that bootstrap supports and overcome the issues already mentioned (align, size, color).

Tests are missing for the new directives, but i will implement as soon as you verify that this on the right track.

[ajoslin]

Hi @bekos! I'll take a look at this and get back to you later today

[ajoslin]

At first look, all these different directives seem like the wrong approach.

I would do it much simpler. There'd be one dropdown-menu directive, which has a toggle attribute or similar:

<dropdown-menu toggle="showDropdown">
  <li ng-repeat="opt in options">{{opt}}</li>
</dropdown-menu>

All the directive would do is transclude whatever's inside, and have the 'click outside to close' logic.

And the template would just be:

<ul class="dropdown-menu" ng-class="{toggle: toggle}" ng-transclude></ul>

You can always add classes to the <dropdown-menu> element and they will be passed to the <ul>.

<dropdown-menu class="pull-right" toggle="showDropdown">

Then all of your directive ideas could be done, eg split button:

<div class="btn-group" ng-class="{toggle: showDropdown}">
  <button class="btn">Hello</button>
  <button class="btn dropdown-toggle" ng-cilck="showDropdown = !showDropdown"><span class="caret"></span></button>
  <dropdown-menu class="pull-right" toggle="showDropdown">
    <li ng-repeat="opt in options">{{opt}}</li>
  </dropdown-menu>
</div>

[shaungrady]

I don't think I'm really seeing the need for a refactor utilizing a transclude and isolate scope. It adds a lot of complexity for what gain? It would no longer function properly with HTML written for Bootstrap CSS/JS. In addition, if you were to have bindings inside your downdown menu, those would get clobbered.

If there's one thing I've learned about directives it's that you should avoid using isolate scopes or transcludes with replacing if at all possible—it makes everything a lot less flexible and a lot more complex.

[bekos]

@ajoslin, @shaungrady Thank you for the time spent on this one.

In fact, the reason i wanted to refactor this one, it was the difficulty to remember the Bootstrap's HTML. In the beggining i was trying to implement just the dropdown-toggle, but i realized that every dropdown in bootstrap share the same logic, so i could implement all the different directives by just changing the template.

I totally agree that @ajoslin 's proposed implementation is much simpler & less complex for the directive, but my target was the directive to be less complex for the user, by just writing:
<dropdown-split-button heading="Hello" size="large"> <li ng-repeat="opt in options">{{opt}}</li> </dropdown-split-button>

In my opinion, this is the reason we transclude & replace <accordion> and <tabs> with <pane> instead of using bootstrap's html and just handle it from outside.

@angular-ui/bootstrap If you feel this should close, please do. I keep no hard feelings :-)

[shaungrady]

@bekos, I understand where you were going with this refactor, but I think it may have drifted away from the original goal a bit. While the refactor looks more clean and simple on the surface, I think it complicates things by forcing you to think about which of the "subdirectives" is the right one to use as well as making you think about the new heading, placement, type, and size attributes. Then, if you want a one-off design for one of the dropdowns, you're out of luck as they all common templates.

The directive as it stands right now may not be the most friendly to implement, I think a lot of flexibility and ease-of-development is afforded to the user in terms of the markup that they build around the dropdown menu.

[joshkurz]

I just want to say that transclusion does not break original bindings by design. I think it's actually good to use it in cases like this.

Sent from my iPhone

On Mar 28, 2013, at 6:34 PM, Shaun Grady [email protected] wrote:

@bekos, I understand where you were going with this refactor, but I think it may have drifted away from the original goal a bit. While the refactor looks more clean and simple on the surface, I think it complicates things by forcing you to think about which of the "subdirectives" is the right one to use as well as making you think about the new heading, placement, type, and size attributes. Then, if you want a one-off design for one of the dropdowns, you're out of luck as they all common templates.

The directive as it stands right now may not be the most friendly to implement, I think a lot of flexibility and ease-of-development is afforded to the user in terms of the markup that they build around the dropdown menu.

—
Reply to this email directly or view it on GitHub.

[bekos]

@shaungrady In my opinion one of the added values for this project is exactly this. You have to remember directives like <pagination> and think of numOfPages, currentPage etc, or <tabs> and remember <pane> , heading etc, or <alert> and type, instead of remembering bootstrap's HTML.
Another case is when we want to change template from bootstrap to foundation like @pkozlowski-opensource has mentioned in another issue. If we leave bootstrap's html as required in the page, this breaks the whole philosophy of just change the template and this works.

It's true that we lose in flexibility compared to writing vanilla HTML, but the case is to provide every option that bootstrap supports, and not edge cases. For example, i want to use letters or latin number in pagination. The directive as it is now cannot support it. Is this a reason to change the implementation and use something like this:

<div class="pagination">
  <ul>
      <page ng-repeat="page in pages" ng-click="moveToPage(page.number)">page.text</page>
  </ul>
</div>

an let user decide the visible pages, text etc?

[ajoslin]

@shaungrady, The problem with the current implementation on master is that it is actually not flexible for different markups - it requires you to have an <a> element as a sibling of a <ul> element. Even for the bootstrap markup, it doesn't support opening a dropdown from a different part of the document.

But you are right, we shouldn't use an isolate scope for this. We could just use $parse to set the is-open attr value to false when the user clicks outside, and throw an error if is-open isn't an assignable expression. And for the class for opening the dropdown visually, we can just toggle the dropdown class in the link function on a $watch(attr.isOpen), and have the class configurable.

@bekos, your extra directive ideas are cool I guess to save a bit of developer time typing things out. But there are so many ways a person could open a dropdown, covering them all with specific directives is definitely going to miss a few cases here and there. Then we have to add more.. and more.. :-)
If we have to add four directives for one functionality, we're doing too much. We can just let the developer write his own directives for his own repetitive cases of dropdown.

[pkozlowski-opensource]

@bekos I must agree with @ajoslin here. We need to balance new vocabulary and flexibility. Some abstractions tend to work better than others and we should strive to find ones that add value and are easy to remember. Personally there is no way I can forget that <alert> is an alert and '' is a tab, but I'm not sure I will be able to remember proposed syntax for dynamic menu.

While the ability to build custom vocabulary is one of the coolest parts of AngularJS we shouldn't push the whole thing to the point where it requires the same amount of typing as bootstrap's HTML+CSS.

Anyway, just sharing my thoughts here.... Keeping this PR open for now so we can brainstorm more.

[bekos]

@ajoslin, @pkozlowski-opensource I'm alone on this one :-)
I don't want to insist anymore, but to make my point clear, I never said that my implementation or the syntax I propose is the best one. I just said that in order for this project to be easier adopted by developers & designers should use custom vocabulary which as @pkozlowski-opensource said is one of the coolest & stronger parts of Angular. And when a developer has an official documentation as @ajoslin proposed, i think it will be easier for someone to look at the API and discover that <dropdown-button> has an attribute size with available values large|small|mini, than to run on bootstrap's site look for the correct HTML form and then on angular's ui site to see how this is handled further.

Anyway, i think that directives should be good enough for use in most cases, otherwise I don't see a point not to use and Bootstrap's Javascript except from the HTML, since at this moment supports more features, like keyboard navigation in the menu.

@pkozlowski-opensource I think this topic is almost exhausted :-) Looking forward for another PR to address the issue that I had, that this directive was more difficult to use in a project compared to the other directives.

[pkozlowski-opensource]

@bekos I've got an impression that there are 2 hard things in this project: positioning issues (especially without jQuery) and API design.

API design is hard but for this PR a consensus seems to be that custom HTML-based DSL is not the way to go. Don't worry about this, we just need to keep trying. I'm sure that one day a good solution will emerge.

Closing for now, let's take the opportunity of #270 to think about this directive some more.