refactor(tabs): adds support for non-fixed tabs #921

robertmesserle · 2014-12-11T23:53:27Z

Closes #825.
Closes #460.

googlebot · 2014-12-11T23:53:27Z

CLAs look good, thanks!

robertmesserle · 2014-12-11T23:56:59Z

src/components/tabs/js/tabsDirective.js

@@ -48,7 +48,8 @@ angular.module('material.components.tabs')
 * @param {integer=} mdSelected Index of the active/selected tab
 * @param {boolean=} mdNoInk If present, disables ink ripple effects.
 * @param {boolean=} mdNoBar If present, disables the selection ink bar.
- * @param {string=}  mdAlignTabs Attribute to indicate position of tab buttons: bottom or top; default is `top`
+ * @param {string=}  mdAlignTabs Attribute to indicate position of tab buttons: `bottom` or `top`; default is `top`
+ * @param {boolean=} mdStretchTabs Attribute to indicate whether or not to stretch tabs: `auto`, `yes`, or `no`; default is `auto`


Options here:

yes: when there is only one page, tabs will stretch to fill the bar (even on desktop)
no: tabs will never stretch to fill the bar
auto: on mobile, single page tab bars will have their tabs stretch to fill the bar

gkalpak · 2014-12-12T09:37:56Z

src/core/services/ripple/ripple.js

@@ -196,7 +194,7 @@ function InkRippleService($window, $timeout) {

      state.animating = true;

-      $timeout(function () {
+      scope.$evalAsync(function () {


Do we need the extra $digest ?

gkalpak · 2014-12-12T10:50:13Z

The commit message does not follow the guidelines btw (unknown type) 😄

robertmesserle · 2014-12-12T19:34:35Z

@gkalpak Great feedback, thanks!

ThomasBurleson · 2014-12-12T19:55:59Z

src/components/tabs/js/inkBarDirective.js

+      if (scope.pagination && scope.pagination.tabData) {
+        var index = tabsCtrl.getSelectedIndex();
+        var data = scope.pagination.tabData.tabs[index] || { left: 0, right: 0, width: 0 };
+        var right = element.parent().prop('offsetWidth') - data.right;


I think this code could be refactored for DRY-ness:

var clazz = (lastIndex > index) ? 'md-transition-left' : (lastIndex < index) ? 'md-transition-right' : 'md-no-transition'; element.css({ left: data.left + 'px', right: right + 'px' }); element.removeClass('md-transition-left md-transition-right md-no-transition'); element.addClass( clazz );

Good call, revised this section of code.

ThomasBurleson · 2014-12-12T19:59:48Z

@robertmesserle - Is this ready for full review and merge into #master ?

robertmesserle · 2014-12-12T20:00:35Z

@ThomasBurleson I believe so. I am still testing this, but I think it's ready for a full review.

ThomasBurleson · 2014-12-12T21:37:41Z

src/components/tabs/js/inkBarDirective.js

+        element
+            .removeClass(classNames.join(' '))
+            .addClass(classNames[classIndex])
+            .css({ left: data.left + 'px', right: right + 'px' });


Concise 👍

The reason I changed this to use percents originally was because it fixed problems when the tabbar or its container is resized.

Resizes can happen arbitrarily, and they won't always trigger a window resize event.

Is there any way we can make this use percentages again to set the ink bar's position?

The size of the container element is now hard-coded, so resizes should not impact this part.

(That is, the outer element stretches as needed, but the inner wrapper is hard-coded to a size far larger than we need to allow for pagination and calculation.)

gkalpak · 2014-12-12T23:17:33Z

src/components/tabs/js/paginationDirective.js

+          filler: 0
+        };
+        //-- store 1-based page number
+        data.page = pages.length === 1 && index === tabs.length - 1


Is there any benefit in having 1-based page indices ? It is kind of confuding in a 0-based world :)

I'm looking into refactoring this now. I initially used 1-based numbers because that was what pagesCount was using; however, I think 0-based is likely the way to go - especially now that pagesCount has been fully factored out.

ajoslin · 2014-12-15T18:22:30Z

src/components/tabs/js/paginationDirective.js

@@ -160,31 +146,108 @@ function TabPaginationDirective($mdConstant, $window, $$rAF, $$q, $timeout) {
      }
    }

+    function stretchTabs() {


Could we actually change this to shouldStretchTabs()? The name stretchTabs() sounds like you're commanding it to do something, when in reality it's just returning a boolean.

ThomasBurleson · 2014-12-16T16:57:16Z

@robertmesserle - ready for me to test and merge into master ?

Closes #825. Closes #460. refactor(tabs): added handling for when the tabs are initially hidden refactor(tabs): switched to 0-based page indexes refactor(tabs): wires up pagination to use $mdMedia rather than a hard-coded pixel size In order to accomplish this, I had to move $mdMedia into its own file in the 'material.core' scope. refactor(tabs): changes per Andrew's feedback Renames `stretchTabs()` to `shouldStretchTabs()` Uses `getElementsByTagName()` in place of `querySelectorAll()` Uses cached version of `angular.element(tabs)` Adds descriptive comment for `data.page` calculation refactor(tabs): moves tabs definition to top of `postLink` method

ajoslin added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Dec 11, 2014

robertmesserle reviewed Dec 11, 2014
View reviewed changes

robertmesserle force-pushed the wip-tab-refactor branch from 938cfe4 to 1fe7e73 Compare December 12, 2014 00:09

gkalpak reviewed Dec 12, 2014
View reviewed changes

robertmesserle changed the title ~~enhance(ripple): adds support for non-fixed tabs~~ refactor(ripple): adds support for non-fixed tabs Dec 12, 2014

robertmesserle force-pushed the wip-tab-refactor branch 3 times, most recently from ff5f516 to 97e392c Compare December 12, 2014 19:33

robertmesserle force-pushed the wip-tab-refactor branch from 97e392c to a999fa7 Compare December 12, 2014 19:47

ThomasBurleson reviewed Dec 12, 2014
View reviewed changes

robertmesserle force-pushed the wip-tab-refactor branch 2 times, most recently from 5f17350 to a2c5961 Compare December 12, 2014 19:59

robertmesserle force-pushed the wip-tab-refactor branch from a2c5961 to f73684e Compare December 12, 2014 21:28

ThomasBurleson reviewed Dec 12, 2014
View reviewed changes

gkalpak reviewed Dec 12, 2014
View reviewed changes

robertmesserle changed the title ~~refactor(ripple): adds support for non-fixed tabs~~ refactor(tabs): adds support for non-fixed tabs Dec 13, 2014

robertmesserle force-pushed the wip-tab-refactor branch 2 times, most recently from 1a89582 to 3964754 Compare December 13, 2014 01:16

ajoslin reviewed Dec 15, 2014
View reviewed changes

robertmesserle force-pushed the wip-tab-refactor branch from ec4a6ac to db1729c Compare December 15, 2014 21:15

ThomasBurleson added the wip label Dec 16, 2014

robertmesserle force-pushed the wip-tab-refactor branch 6 times, most recently from e269160 to 8da345b Compare December 16, 2014 22:39

robertmesserle force-pushed the wip-tab-refactor branch from 8da345b to a7f7ff8 Compare December 16, 2014 22:40

robertmesserle closed this in 6397040 Dec 18, 2014

ajoslin removed the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Dec 18, 2014

robertmesserle deleted the wip-tab-refactor branch January 8, 2015 19:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(tabs): adds support for non-fixed tabs #921

refactor(tabs): adds support for non-fixed tabs #921

robertmesserle commented Dec 11, 2014

googlebot commented Dec 11, 2014

robertmesserle Dec 11, 2014

gkalpak Dec 12, 2014

gkalpak commented Dec 12, 2014

robertmesserle commented Dec 12, 2014

ThomasBurleson Dec 12, 2014

robertmesserle Dec 12, 2014

ThomasBurleson commented Dec 12, 2014

robertmesserle commented Dec 12, 2014

ThomasBurleson Dec 12, 2014

ajoslin Dec 15, 2014

robertmesserle Dec 15, 2014

gkalpak Dec 12, 2014

robertmesserle Dec 12, 2014

ajoslin Dec 15, 2014

ajoslin Dec 15, 2014

ThomasBurleson commented Dec 16, 2014

refactor(tabs): adds support for non-fixed tabs #921

refactor(tabs): adds support for non-fixed tabs #921

Conversation

robertmesserle commented Dec 11, 2014

googlebot commented Dec 11, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Dec 12, 2014

robertmesserle commented Dec 12, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomasBurleson commented Dec 12, 2014

robertmesserle commented Dec 12, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomasBurleson commented Dec 16, 2014