feat(tabs): paginate tab header when exceeds width #2084

andrewseguin · 2016-12-05T21:35:04Z

No description provided.

crisbeto · 2016-12-05T21:44:24Z

src/lib/tabs/tab-header.ts

+  encapsulation: ViewEncapsulation.None,
+  host: {
+    '[class.md-tab-header]': 'true',
+    '[class.scroll]': '_isScrollingEnabled()',


One thing to bear in mind here: the _isScrollingEnabled will be run on every change detection cycle. Since it uses offsetWidth and scrollWidth to determine whether the element is scrollable, it will end up causing a lot of layout recalculations. This was an issue in Material 1 and it might be better to address it earlier for Material 2.

This is a very good point. We should recalculate this as infrequently as possible.

I agree that this is not ideal. Do you have any suggestions on how to reduce the amount of times we call this but still have an up to date pagination? It seems that checking this in ngAfterViewChecked will be a little better but still way more than we should need.

I think that caching the width and only updating it in the following cases should be fine:

When initialized.

When a tab gets added/removed.

When a tab's heading changes.

The third one might be tricky to get right since we'd have to either get the innerText from the DOM on every change detection (which apparently also causes a reflow), or use a MutationObserver per tab. We could also try to have have one "main" MutationObserver on the header, but I don't know whether that won't end up firing too often, because AFAIK, you need to enable a bunch of flags to get it working.

Yeah, I chatted with Andrew yesterday about this and that's pretty much example what we're doing. MutationObserver per-tab, going to do that part in a follow-up PR (and just handle init, adding/removing tabs in this PR).

Agreed, I think init and tabs add/remove should handle the vast majority of use cases. I'll include just that logic in this PR.

In another PR, I'll attempt to check tab heading changes (text node modifications) using the MutationObserver.

I think one more thing to consider is that we could check when a change is made to the selected tab or focused tab. This would provide a fallback that could be triggered by the user who may try to get the header to update the scrolling after some event has caused the width to change.

Talked with Jeremy - adding a check to select/focus will probably be overkill. But adding a check on resize will probably be nice.

Also, Jeremy had a good idea to make the mutation observer logic as a separate directive that could be placed into core for other components that need similar checks.

I like the idea of the MutationObserver directive, although isn't it basically 10 lines of boilerplate anyway?

I figure it will be a nice convenience if we ever need to do something like this again

jelbourn · 2016-12-05T22:16:59Z

src/demo-app/tabs/tabs-demo.scss

-  }
+}
+
+.dynamic-tabs-demo {


.demo-dynamic-tabs

jelbourn · 2016-12-05T22:17:45Z

src/lib/tabs/tab-group.spec.ts

-      ]
+      ],
+      providers: [
+        { provide: Dir, useFactory: () => { return {value: 'ltr'}; }


Why not put a real dir directive above the tabs?

jelbourn · 2016-12-05T22:23:47Z

src/lib/tabs/tab-header.html

@@ -0,0 +1,23 @@
+<div class="md-tab-label-pagination before md-elevation-z4"


What's this before class?

jelbourn · 2016-12-05T22:24:49Z

src/lib/tabs/tab-header.html

+<div class="md-tab-label-pagination before md-elevation-z4"
+     aria-hidden="true"
+     md-ripple [md-ripple-disabled]="_disableScrollBefore"
+     [class.disabled]="_disableScrollBefore"


class.md-tab-header-disabled?

jelbourn · 2016-12-05T22:26:25Z

src/lib/tabs/tab-header.ts

+
+export type ScrollDirection = 'after' | 'before';
+
+@Component({


Needs a class description.

jelbourn · 2016-12-05T22:42:17Z

src/lib/tabs/tab-header.ts

+    return this._dir && this._dir.value === 'rtl' ? 'rtl' : 'ltr';
+  }
+
+  _updateScrollPosition() {


These scrolling methods could use some descriptions

jelbourn · 2016-12-05T22:42:30Z

src/lib/tabs/tab-header.ts

+    }
+
+    this._renderer.setElementStyle(this._tabList.nativeElement,
+        'transform', `translate3d(${translateX}, 0, 0)`);


See the applyCssTransform function

jelbourn · 2016-12-05T22:44:24Z

src/lib/tabs/tab-header.ts

+      this.scrollDistance -= viewLength / 3;
+    } else {
+      this.scrollDistance += viewLength / 3;
+    }


this.scrollDistance = (scrollDir == 'before' ? -1 : 1) * viewLength / 3;

?

Also, explain where this calculation comes from?

jelbourn · 2016-12-05T22:45:11Z

src/lib/tabs/tab-header.ts

+
+    // If move is required, overscroll by a small amount to provide an affordance to click the
+    // next label.
+    const exaggeratedOverscroll = 60;


Make this a file-level constant?

jelbourn · 2016-12-05T22:46:08Z

src/lib/tabs/tab-header.ts

+    const selectedLabel = this._labelWrappers.toArray()[labelIndex];
+    const viewLength = this._tabListContainer.nativeElement.offsetWidth;
+
+    let labelBeforePos: number, labelAfterPos: number;


Comment explaining what these variables contain?

andrewseguin · 2016-12-06T21:32:40Z

Shuffled the logic so that the majority of scrolling stuff is evaluated on content check and checks are only made conditionally (e.g. tab list changed, selected index changed, etc)

Really appreciate the reviews, the amount of layout reflows has dropped significantly.

jelbourn

LGTM, one last nit

jelbourn · 2016-12-07T20:42:08Z

src/lib/tabs/tab-header.ts

+  /**
+   * Waits one frame for the view to update, then updates the ink bar and scroll.
+   * Note: This must be run outside of the zone or it will create an infinite change detection loop
+   * TODO: internal


No need for TODO: internal any more

andrewseguin · 2016-12-07T20:55:29Z

Removed the internal comment.

I'd like to add a reference drawing into the folder to help people understand better about how the element container widths/offsets work. Here's the WIP: https://docs.google.com/drawings/d/1fhtiM-NabWhKP8pVsz8hCAQkzWz1QXrUG0LH-xQdpqI/edit?usp=sharing

mmalerba · 2016-12-07T23:32:32Z

Property {'_disableScrollBefore', '_disableScrollAfter', '_handleKeydown'} is private and only accessible within class 'MdTabHeader'

andrewseguin · 2016-12-08T18:13:46Z

Okay, should be good to merge!

angular-automatic-lock-bot · 2019-09-06T05:01:46Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 5, 2016

andrewseguin added pr: needs review and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Dec 5, 2016

andrewseguin assigned jelbourn Dec 5, 2016

crisbeto reviewed Dec 5, 2016

View reviewed changes

andrewseguin force-pushed the tabs-pagination branch from 41eda70 to f7a97de Compare December 5, 2016 22:25

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 5, 2016

jelbourn reviewed Dec 5, 2016

View reviewed changes

andrewseguin force-pushed the tabs-pagination branch from 1237037 to cfcbdbc Compare December 6, 2016 21:30

jelbourn reviewed Dec 7, 2016

View reviewed changes

jelbourn added pr: lgtm and removed pr: needs review labels Dec 7, 2016

andrewseguin force-pushed the tabs-pagination branch from 875a8e4 to 4ee83d9 Compare December 7, 2016 20:50

jelbourn added the action: merge The PR is ready for merge by the caretaker label Dec 7, 2016

mmalerba added action: merge The PR is ready for merge by the caretaker pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Dec 7, 2016

andrewseguin added 9 commits December 8, 2016 10:06

initial implementation of scrolling

56090b9

add scrolling when clicking on tab

1e42bd3

fix rtl

dc6f6d5

create component for header

4c4363d

rebase

4a32f5e

support stretch

528c9b8

do not focus() the selected index

15f710c

be sure to scroll to selected index after view

846de6e

be sure to scroll to selected index after view coment

4df4bb0

andrewseguin added 11 commits December 8, 2016 10:08

fix tests

6713684

cleanup dynamic tab demo

4431434

fix tests, make Dir optional

c1d6f82

remove Dir import in spec

b9ff83f

restore html

565a6db

remove index to focus

b45f9bc

reduce the amount of layout reflows

d85dbee

fix test for IE/Safari by using minwidth

61ab3d0

fix lint

e2ff7c2

remove internal TODO comment

6cd6d34

remove private on functions/variables used in template

1b29725

andrewseguin force-pushed the tabs-pagination branch from 5e5122f to 1b29725 Compare December 8, 2016 18:09

andrewseguin added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Dec 8, 2016

andrewseguin added 2 commits December 8, 2016 10:29

fix tests for latest ripple changes

c6a259b

remove double quotes

9d743f6

mmalerba merged commit 92e26d7 into angular:master Dec 8, 2016

andrewseguin deleted the tabs-pagination branch December 8, 2016 19:11

andrewseguin mentioned this pull request Dec 9, 2016

[Tabs] md-tab-group scrolling/pagination missing #769

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(tabs): paginate tab header when exceeds width #2084

feat(tabs): paginate tab header when exceeds width #2084

andrewseguin commented Dec 5, 2016

crisbeto Dec 5, 2016

jelbourn Dec 5, 2016

andrewseguin Dec 5, 2016

crisbeto Dec 6, 2016 •

edited

Loading

jelbourn Dec 6, 2016

andrewseguin Dec 6, 2016

andrewseguin Dec 6, 2016

crisbeto Dec 6, 2016

jelbourn Dec 6, 2016

jelbourn Dec 5, 2016

jelbourn Dec 5, 2016

jelbourn Dec 5, 2016

jelbourn Dec 5, 2016

jelbourn Dec 5, 2016

jelbourn Dec 5, 2016

jelbourn Dec 5, 2016

jelbourn Dec 5, 2016

jelbourn Dec 5, 2016

jelbourn Dec 5, 2016

andrewseguin commented Dec 6, 2016

jelbourn left a comment

jelbourn Dec 7, 2016

andrewseguin commented Dec 7, 2016

mmalerba commented Dec 7, 2016

andrewseguin commented Dec 8, 2016

angular-automatic-lock-bot bot commented Sep 6, 2019

		@@ -0,0 +1,23 @@
		<div class="md-tab-label-pagination before md-elevation-z4"


		export type ScrollDirection = 'after' \| 'before';

		@Component({

feat(tabs): paginate tab header when exceeds width #2084

feat(tabs): paginate tab header when exceeds width #2084

Conversation

andrewseguin commented Dec 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto Dec 6, 2016 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewseguin commented Dec 6, 2016

jelbourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewseguin commented Dec 7, 2016

mmalerba commented Dec 7, 2016

andrewseguin commented Dec 8, 2016

angular-automatic-lock-bot bot commented Sep 6, 2019

crisbeto Dec 6, 2016 •

edited

Loading