refactor(tabs): Change syntax, add features #287
Conversation
|
Still need to add pane-group ability as per #160. but we should probably wait until the dropdown is refactored for that. |
|
Let's discuss it in that issue, I'm not sure about that one. |
|
Sorry for all the re-commits and ci notifications, people :-) I keep finding little things to add tests for & cases I didn't think of. |
|
More tests are always good. :) Sent from my iPhone On Mar 31, 2013, at 9:34 AM, Andy Joslin notifications@github.com wrote:
|
|
@ajoslin I had a quick look and it looks really good, nice and clean. I'm not sure I can catch more by reviewing code. @angular-ui/bootstrap would be awesome to have another look and merge it so this change makes it to a next release. @ajoslin I would only change a commit message to automatically close all the GH issues. We will also have to communicate clearly to indicate that this is massively breaking change :-) |
|
There is a bug when for example there are nested tabs. I think this has to do with the "Only the active tab's content is actually ever in the DOM" feature, but @ajoslin knows better :-) It will be nice to have this one into the next release, as it definitely makes the library better. |
|
Yes you're right! I think the tabs always lose their state when you switch. I will have to put it back to the way it used to be. |
|
could we just use the scope delegate '&' on the tabset to provide a callback for tab changes. like scope {
postSelect: '&'
}it seems like this would be a lot cleaner, and avoid the $eval. |
|
Ok, fixed the problem with tabs losing their state, and added tests for it! Thanks @bekos! Was easier than I thought it would be. When you switch tabs, it now just removes the old tab element from the DOM instead of setting the contents' html to an empty string. @FAQinghere - Doesn't using the '&' binding require the user to define a callback? I didn't think the scope.$eval was that messy, it's a one-liner after all. |
|
@pkozlowski-opensource Perhaps for version 0.3.0 we should add in a Something like: angular.module('ui.bootstrap.tabs').directive('tabs', function() {
return function() {
throw new Error("The `tabs` directive is deprecated, please migrate to `tabset`. Instructions can be found at http://github.com/angular-ui/bootstrap/tree/master/CHANGELOG.md#0.3.0-tabs");
};
}); |
| if (!tabs.length) { | ||
| ctrl.select(tab); | ||
| } | ||
| tabs.push(tab); |
There was a problem hiding this comment.
It would seem safer to push the new tab before selecting it?
There was a problem hiding this comment.
Good point. fixed :-)
|
OK, implemented all of Pete's changes, as well as some general documentation polish. Look good to you, Pete? |
|
Updated the commit message with a BREAKING CHANGE section in the footer, according to the Git Commit Message Conventions doc :-) |
|
I love the functional changes, in general, but I think (I'll give it a little time today or tomorrow) that the implementation could be significantly simpler. |
| if(selected) { | ||
| tabsCtrl.select(scope); | ||
| controller: ['$scope', function TabCtrl($scope) { | ||
| this.getHeadingElement = function() { |
There was a problem hiding this comment.
Why is this function here? If we are adding the heading element to the scope anyway, should it not be available to child directives anyway?
|
@FAQinghere if you can figure out how to make tab-heading a directive, do it! It'd be better. The problem was by the time the tab-heading directive got compiled, it was already transcluded into the content area and had no idea what tab it was part of. I tried a lot of things... @petebacondarwin Good point, that was old. removed that :-) |
|
Sorry, I got pulled into another project and don't have the time to follow up on this right now, but I'll make a few notes. I think it would be better to make the mark up more explicit. It would make it easier to clean up the implementation, and also make it more parallel to standard markup. example: <tabset>
<tab>
<tabhead></tabhead>
<tabpane></tabpane>
</tab>
</tabset>We could use the delegating scope variable '&' to provide a simple callback to the outer scope if desired (using an attribute such as on-nav:"changedTab(tabIndex, tabName)". We could use restrict: "EA" on all the directives. When we need to capture the inner content (on tabhead, because it actually belongs outside the tab, on the tabset, in terms of final markup), we can use 'replace' internally to restrict it to the tag form (might need a separate tag name internally because Angular doesn't recognize that the tag is being replaced and will think it's getting redundant copies of the tag and throw an error). Then we can easily pull the inner content from the tabhead using el.find('customtag'), avoiding the need to process both tags and attributes, even though the original markup can make use of both. We could eliminate 'heading' attribute altogether, as users could use the tabhead directive. On the other hand, we might add in a name attribute on the tag, which could be passed to the outer callback if provided, as mentioned above. Looking at the current implementation, I think this could add functionality and decrease the code considerably. |
|
I think the thing is that for many cases people don't want complex headings On 10 April 2013 15:13, FAQinghere notifications@github.com wrote:
|
|
I thought about that too. In that case, we could use the name (or 'heading', etc) attribute to build the tabhead in cases where one isn't given. Either way the dom has to be manipulated manually for the tabhead because it belongs at a higher level in the final html. That still leaves the 'tab-pane' directive open to debate of course. On the other hand, if Angular's goal is to teach HTML new tricks, it seems to make sense that the markup used should be as HTML-like as possible. A tiny bit of additional markup isn't evil if it makes the underlying implementation cleaner and more robust. |
|
@FAQinghere we are trying to avoid super verbose markup like that - although it would fix some of the problems, we don't want to return to the ultra-verbosity of twitter bootstrap's own js/markup :-) |
|
@angular-ui/bootstrap do you guys think this is this good to merge? |
|
@ajoslin Thanks a lot for your work on this refactor. Also, it looks like the <tabset> element isn't replaced by a <div>; it's left as-is in the output HTML. |
|
Thanks @Nandan108 - I'll look into this. I may just give up on having only the active tab's element in the DOM :-) |
|
Just a note - I am working on this but have had a bit of busyness lately (moving). Tonight or early tomorrow I'll have the refactor done - just gotta finish changing the unit tests. Hopefully we can get it pushed with 0.3.0 soon 😄 |
|
Awesome @ajoslin And don't stress about it :-) |
|
OK, fixed tabs to ng-repeat all the tabs and display only the active ones now! Also tests are more awesome, and I rebased it onto master. |
|
Seems like this is all working. At least, it is for me. |
|
It's too late for that :-) Let's merge it into master though! Everyone good with that @angular-ui/bootstrap ? |
|
I wanted to merge it yesterday alongside with other PRs but for some reason CI server is stuck on git pooling: Guess it died so we can't run tests on all the browsers :-( |
|
Weird! Have you e-mailed Igor? I will and cc everyone if not. |
|
@ajoslin nope, I didn't sorry, trying to push next chapter of the book today, totally forgotten :-( I saw it handing previously and saw some info on the net regarding Jenkins + git problems. We should probably ask Igor to configure a hook so the build is started by push to GitHub and not based on pooling. I think it is set up like this for the AngularJS project. |
|
+1 :) |
|
Desperate for this. When do you think we'll get it? Tks for the hard work. |
|
@angular-ui/bootstrap ready to merge this? :-) CI issues are fixed. I will squash and rebase against master and push again to be sure. |
* Rename 'tabs' directive to 'tabset', and 'pane' directive to 'tab'. The new syntax is more intuitive; The word pane does not obviously represent a subset of a tab group. (Closes #186) * Add 'tab-heading' directive, which is a child of a 'tab'. Allows HTML in tab headings. (Closes #124) * Add option for a 'select' attribute callback when a tab is selected. (Closes #141) * Tabs transclude to title elements instead of content elements. Now the ordering of tab titles is always correct. (Closes #153) BREAKING CHANGE: The 'tabs' directive has been renamed to 'tabset', and the 'pane' directive has been renamed to 'tab'. To migrate your code, follow the example below. Before: <tabs> <pane heading="one"> First Content </pane> <pane ng-repeat="apple in basket" heading="{{apple.heading}}"> {{apple.content}} </pane> </tabs> After: <tabset> <tab heading="one"> First Content </tab> <tab ng-repeat="apple in basket" heading="{{apple.heading}}"> {{apple.content}} </tab> </tabset>
|
@ajoslin let's merge it! I haven't had time to review it but I'm sure that you did great job. And we can always fix things is something goes wrong! |
|
Landed in c532659 :-) |
|
So... how is the "select" attribute going? do we have this feature on master? or did anyone get a workarround to get notified on tab selection? Sorry i did not know the proper place to ask this :_) |
|
This is already on master! |
|
Glad to hear it... pane select="callback" ? |
<tab select="callback()"> |
|
Mmmm, the new tabset tab structure doesn't seem to work for me, i downloaded a snapshot of 0.4... should i clone master and build the js myself? how do i build it? Sorry, i know those are noob questions... but that's what i am right now :_) |
|
Download from github is not enough, you have to use grunt to build the files. |
|
Grunt, ok, thank you man! |
|
Is this part of 0.3.0? I downloaded the build and it looks like it still has the old tabs and panes directives. |
|
@chiester nope, it will be only released as part of 0.4.0 |
|
Can I build it with the new tabs component? |
|
@chiester just build the latest master, it should be pretty stable. |
|
I was specifically looking for the new feature in tabs where it allows a callback on select. Thanks. |
Show x icon to clear/reset the current value (like select2)
<tabs>directive to<tabset>, and directive<pane>to<tab>.The new syntax is more intuitive; The word pane does not obviously
represent a subset of a tab group. (Closes (WIP) refactor(tabs): Rework tabs to transclude to the headings #186)
<tab-heading>directive, which is a child of a<tab>. AllowsHTML in tab headings. (Closes Support HTML in tabs title #124)
the so the ordering is always correct. (Closes fix(tabs): keep stable tab order (when filtering ng-repeat) #153)
an unintended bonus of transcluding to title elements, and improves
performance.
BREAKING CHANGE: The
<tabs>directive has been renamed to<tabset>, andthe
<pane>directive has been renamed to<tab>.