Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

(WIP) refactor(tabs): Rework tabs to transclude to the headings #186

Closed
wants to merge 2 commits into from
Closed

(WIP) refactor(tabs): Rework tabs to transclude to the headings #186

wants to merge 2 commits into from

Conversation

ajoslin
Copy link
Contributor

@ajoslin ajoslin commented Mar 4, 2013

Refactored tabs to transclude themselves into the headings instead of the content.

This fixes problems with ng-repeat causing tab headings to appear in the wrong order, since now the headings reflect the real elements, not the contents.

As a bonus, we also now only load the content of the visible tab into the DOM.

I just need to add a lot more unit tests.

@ajoslin
Copy link
Contributor Author

ajoslin commented Mar 4, 2013

We could look at #160 and #163

petebacondarwin
petebacondarwin reviewed Mar 4, 2013
View reviewed changes
src/tabs/docs/demo.html
@@ -2,9 +2,13 @@
<tabs>
<pane heading="Static title">Static content</pane>
<pane ng-repeat="pane in panes" heading="{{pane.title}}" active="pane.active">{{pane.content}}</pane>
<pane>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we could a different set of names?

<tabset>
  <tab>
    <tab-heading>
    <tab-pane>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dunno, that seems a bit too verbose to me. @pkozlowski-opensource what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be clear:

  • tabs -> tabset
  • pane -> tab
  • pane-heading -> tab-heading
  • ... -> tab-pane (or tab-body)

So the nice thing then is that everything starts with tab (or bs-tab) so it is easy to see that they are all related (you could even go for tab-set for super convergence). The only additional item is tab-pane/body, which I can live without but just thought it made the mark-up clearer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeh, I agree with @ajoslin that it slowly starts to resemble bootstrap's HTML :-) Especially if we want to to add tabs grouping in the future.

It is a funny story with this tabs directive, I thought that it will be used for simple cases only where people don't want to be bothered writing markup by hand. But now it feels like this is one of the most popular directives lol
I guess what I'm trying to say here is that we shouldn't make directives syntax more complex / verbose as compared to bootstrap's markup.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

<tabset>
  <tab heading="Static title">Static content</tab>
  <tab ng-repeat="tab in tabs" heading="{{tab.title}}" active="tab.active">{{tab.content}}</tab>
  <tab>
    <tab-heading><b>HTML</b> in a <i>heading?</i></tab-heading>
    This is cool!
  </tab>
</tabset>

Now that is not so verbose is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That does actually make a little more sense than <pane> - and on the topic of verbosity, tab is one less character of typing than pane 😆

And since we're transcluding to the tab now, <tab> actually makes more sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I agree we should switch to Pete's proposed syntax - as it makes everything start with tab. @angular-ui/bootstrap any objections?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bump so should we go with new syntax?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 !

@petebacondarwin petebacondarwin mentioned this pull request Mar 4, 2013
Closed
@ajoslin
Copy link
Contributor Author

ajoslin commented Mar 6, 2013

So I committed with a broken unit test. It's got me stumped!

In the latest version on tabs.js line 80, I transclude the paneContent to the parent scope. This makes it so it evaluates the pane content against the original scope instead of the new isolate scope.

However, this breaks the heading binding! My guess is the transclude to the parent on line 80 causes the {{heading}} binding to start evaluating on the parent scope too, causing it to be wrong. Any ideas? Would someone mind giving this a look when they have a chance?

@petebacondarwin
Copy link
Member

Will take a look today

On 6 March 2013 15:36, Andy Joslin [email protected] wrote:

So I committed with a broken unit test. It's got me stumped! In the latest
version on tabs.js line 80 that I transclude the paneContent to the parent
scope, so it evaluates the tab content against the original scope instead
of the new isolate scope.

However, this breaks the heading binding! My guess is the transclude to
the parent on line 80 causes the {{heading}} binding to start evaluating
on the parent scope too, causing it to be wrong. Any ideas? Would someone
mind giving this a look when they have a chance?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/186#issuecomment-14505460
.

@ajoslin
Copy link
Contributor Author

ajoslin commented Mar 18, 2013

Has anyone had a chance to look at this? @angular-ui/bootstrap

@ajoslin
Copy link
Contributor Author

ajoslin commented Mar 28, 2013

Closing for now - going to refactor it one more time with renaming the directives, and also to fix weird transclusion bugs everywhere.

@ajoslin ajoslin closed this Mar 28, 2013
@ajoslin ajoslin mentioned this pull request Mar 30, 2013
Closed
ajoslin added a commit that referenced this pull request Mar 30, 2013
@ajoslin
refactor(tabs): Change syntax, add features
d53855f 
* Tabs transclude to title elements instead of content elements, so the
ordering is always correct (#153)
* Rename `<tabs>` to `<tabset>`, `<pane>` to `<tab>` (#186)
* Add `<tab-heading>` directive, which is a child of a `<tab>`. Allows
HTML in tab headings (#124)
* Add `select` attribute callback when tab is selected (#141)
* Only the active tab's content is actually ever in the DOM
ajoslin added a commit that referenced this pull request Mar 31, 2013
@ajoslin
refactor(tabs): Change syntax, add features
5b4ed04 
* Tabs transclude to title elements instead of content elements, so the
ordering is always correct (#153)
* Rename `<tabs>` to `<tabset>`, `<pane>` to `<tab>` (#186)
* Add `<tab-heading>` directive, which is a child of a `<tab>`. Allows
HTML in tab headings (#124)
* Add `select` attribute callback when tab is selected (#141)
* Only the active tab's content is actually ever in the DOM
ajoslin added a commit that referenced this pull request Apr 6, 2013
@ajoslin
refactor(tabs): Change syntax, add features
809b92c 
* Tabs transclude to title elements instead of content elements, so the
ordering is always correct (Closes #153)
* Rename `<tabs>` to `<tabset>`, `<pane>` to `<tab>` (Closes #186)
* Add `<tab-heading>` directive, which is a child of a `<tab>`. Allows
HTML in tab headings (Closes #124)
* Add `select` attribute callback when tab is selected (Closes #141)
* Only the active tab's content is now actually ever in the DOM
ajoslin added a commit that referenced this pull request Apr 8, 2013
@ajoslin
feat(tabs): Change directive name, add features
7d01892 
* 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)
* Only the active tab's content is ever present in the DOM. This is
 another plus of transcluding tabs to title elmements, and provies a
 performance increase.

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 added a commit that referenced this pull request Apr 9, 2013
@ajoslin
feat(tabs): Change directive name, add features
8ee76b4 
* 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)
* Only the active tab's content is ever present in the DOM. Provides an
 increase in performance.

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 added a commit that referenced this pull request Apr 28, 2013
@ajoslin
feat(tabs): Change directive name, add features
5a0bd33 
* 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 added a commit that referenced this pull request May 9, 2013
@ajoslin
feat(tabs): Change directive name, add features
bfe20e9 
* 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 added a commit that referenced this pull request May 9, 2013
@ajoslin
feat(tabs): Change directive name, add features
c532659 
* 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>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ajoslin @petebacondarwin @pkozlowski-opensource