bug(tabs): fix arrows not showing on reszize #6303 #6304

amcdnl · 2017-08-06T15:03:38Z

Addresses #6303

crisbeto · 2017-08-06T15:35:22Z

src/lib/tabs/tab-header.ts

@@ -387,6 +388,7 @@ export class MdTabHeader extends _MdTabHeaderMixinBase
   * This is an expensive call that forces a layout reflow to compute box and scroll metrics and
   * should be called sparingly.
   */
+  @HostListener('window:resize')


We don't really use the @HostListener anywhere. Can you switch to using the host property on the component definition instead?

Adds the `HostBinding` and `HostListener` decorators to the banned symbols list for consistency's sake. Refactors the handful of places that were using them. Relates to angular#6304.

jelbourn · 2017-08-07T16:36:32Z

src/lib/tabs/tab-header.ts

@@ -79,6 +80,7 @@ export const _MdTabHeaderMixinBase = mixinDisableRipple(MdTabHeaderBase);
    'class': 'mat-tab-header',
    '[class.mat-tab-header-pagination-controls-enabled]': '_showPaginationControls',
    '[class.mat-tab-header-rtl]': "_getLayoutDirection() == 'rtl'",
+    '(window:resize)': "_checkPaginationEnabled()"


At the very least this call should be debounced. The comment for the handler even says "This is an expensive call that forces a layout reflow to compute box and scroll metrics and should be called sparingly."

jelbourn · 2017-08-07T16:37:54Z

src/lib/tabs/tab-header.ts

@@ -79,6 +80,7 @@ export const _MdTabHeaderMixinBase = mixinDisableRipple(MdTabHeaderBase);
    'class': 'mat-tab-header',
    '[class.mat-tab-header-pagination-controls-enabled]': '_showPaginationControls',
    '[class.mat-tab-header-rtl]': "_getLayoutDirection() == 'rtl'",
+    '(window:resize)': "_checkPaginationEnabled()"


We should really just have one library-wide resize handler instead of each component adding their own. Add a TODO?

jelbourn · 2017-08-07T16:38:14Z

src/lib/tabs/tab-header.ts

@@ -79,6 +80,7 @@ export const _MdTabHeaderMixinBase = mixinDisableRipple(MdTabHeaderBase);
    'class': 'mat-tab-header',
    '[class.mat-tab-header-pagination-controls-enabled]': '_showPaginationControls',
    '[class.mat-tab-header-rtl]': "_getLayoutDirection() == 'rtl'",
+    '(window:resize)': "_checkPaginationEnabled()"


Any way to add a unit test for this?

Adds the `HostBinding` and `HostListener` decorators to the banned symbols list for consistency's sake. Refactors the handful of places that were using them. Relates to #6304.

amcdnl · 2017-08-07T21:54:25Z

@jelbourn - Ready for another review.

jelbourn

LGTM, just a couple minor things. Add the "merge-ready" label when ready

jelbourn · 2017-08-08T01:06:50Z

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

+      tick(10);
+      fixture.detectChanges();
+
+      expect(header._checkPaginationEnabled).toHaveBeenCalled();


FYI, I generally prefer tests that assert on the outcome of calling the function rather than using a spy. In this case, though, it would end up being a brittle geometric value so the spy is okay.

Ya, thats what I was thinking too.

jelbourn · 2017-08-08T01:08:08Z

src/lib/tabs/tab-header.ts

@@ -143,6 +146,11 @@ export class MdTabHeader extends _MdTabHeaderMixinBase
              private _changeDetectorRef: ChangeDetectorRef,
              @Optional() private _dir: Directionality) {
    super();
+
+    // TODO: Add library level window listener https://goo.gl/FJWhZM


I think you got the wrong comment in your shortlink
(probably meant #6304 (comment))

jelbourn · 2017-08-08T01:08:50Z

src/lib/tabs/tab-header.ts

@@ -78,7 +78,7 @@ export const _MdTabHeaderMixinBase = mixinDisableRipple(MdTabHeaderBase);
  host: {
    'class': 'mat-tab-header',
    '[class.mat-tab-header-pagination-controls-enabled]': '_showPaginationControls',
-    '[class.mat-tab-header-rtl]': "_getLayoutDirection() == 'rtl'",
+    '[class.mat-tab-header-rtl]': "_getLayoutDirection() == 'rtl'"


TrailingCommas4Life

Are you serial?!

jelbourn · 2017-08-08T01:10:28Z

Actually I just noticed that this breaks the prerender task. The resize event should only be setup when on the browser platform

amcdnl · 2017-08-08T21:37:48Z

@jelbourn - ready for another review.

In angular#6304, an extra window resize handler was added in order to call `_checkPaginationEnabled`. This handler is unnecessary, because we have another resize handler a little bit below that calls `_updatePagination`, which will call `_checkPaginationEnabled` internally. The original issue that angular#6304 was fixing was due to the fact that the current listener wasn't being run in the Angular zone. These changes remove the extra listener, increase the debounce interval and move the handler outside back into the zone. There may be a slight overhead from moving the listener into the zone, however it should be offset by not having another listener and not calling `_checkPaginationEnabled` twice.

In #6304, an extra window resize handler was added in order to call `_checkPaginationEnabled`. This handler is unnecessary, because we have another resize handler a little bit below that calls `_updatePagination`, which will call `_checkPaginationEnabled` internally. The original issue that #6304 was fixing was due to the fact that the current listener wasn't being run in the Angular zone. These changes remove the extra listener, increase the debounce interval and move the handler outside back into the zone. There may be a slight overhead from moving the listener into the zone, however it should be offset by not having another listener and not calling `_checkPaginationEnabled` twice.

angular-automatic-lock-bot · 2019-09-06T21:02:00Z

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.}

bug(tabs): fix arrows not showing on reszize angular#6303

b185eac

amcdnl self-assigned this Aug 6, 2017

amcdnl requested a review from jelbourn August 6, 2017 15:03

amcdnl requested a review from andrewseguin as a code owner August 6, 2017 15:03

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 6, 2017

crisbeto reviewed Aug 6, 2017

View reviewed changes

crisbeto mentioned this pull request Aug 6, 2017

build: disallow usage of HostBinding and HostListener #6307

Merged

chore(*): update to use host vs hostliener

b84092c

jelbourn reviewed Aug 7, 2017

View reviewed changes

bug(tabs): address feedback in pr

68e6ddd

jelbourn approved these changes Aug 8, 2017

View reviewed changes

jelbourn added the pr: lgtm label Aug 8, 2017

bug(tabs): address PR feedback

a792f6e

jelbourn added the action: merge The PR is ready for merge by the caretaker label Aug 8, 2017

mmalerba merged commit f96ffeb into angular:master Aug 9, 2017

amcdnl deleted the tab-arrows-resize branch August 12, 2017 14:06

crisbeto mentioned this pull request Aug 12, 2017

perf(tabs): avoid extra resize handler and reflow #6434

Merged

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

bug(tabs): fix arrows not showing on reszize #6303 #6304

bug(tabs): fix arrows not showing on reszize #6303 #6304

amcdnl commented Aug 6, 2017

crisbeto Aug 6, 2017 •

edited

Loading

amcdnl Aug 6, 2017 •

edited

Loading

jelbourn Aug 7, 2017

jelbourn Aug 7, 2017

jelbourn Aug 7, 2017

amcdnl commented Aug 7, 2017

jelbourn left a comment

jelbourn Aug 8, 2017

amcdnl Aug 8, 2017

jelbourn Aug 8, 2017

jelbourn Aug 8, 2017

amcdnl Aug 8, 2017

jelbourn commented Aug 8, 2017

amcdnl commented Aug 8, 2017

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

bug(tabs): fix arrows not showing on reszize #6303 #6304

bug(tabs): fix arrows not showing on reszize #6303 #6304

Conversation

amcdnl commented Aug 6, 2017

crisbeto Aug 6, 2017 • edited Loading

Choose a reason for hiding this comment

amcdnl Aug 6, 2017 • 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

amcdnl commented Aug 7, 2017

jelbourn left a 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

jelbourn commented Aug 8, 2017

amcdnl commented Aug 8, 2017

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

crisbeto Aug 6, 2017 •

edited

Loading

amcdnl Aug 6, 2017 •

edited

Loading