feat(material-experimental/mdc-tabs): add option to fit ink bar to content #17507

andrewseguin · 2019-10-25T17:13:13Z

See material components documentation for an example of the ink bar fitting to the tab label content

https://material-components.github.io/material-components-web-catalog/#/component/tabs

mmalerba · 2019-10-25T17:43:59Z

src/material-experimental/mdc-tabs/ink-bar.ts

  private _createIndicator(document: Document) {
+    this._indicator = document.createElement('span');


The name of this class seems a little strange, it's easy to confuse with the concept of a Foundation in MDC. (I know it wasn't created in this PR, but just something I noticed now).

Why does this class do so much direct DOM manipulation as opposed to being its own component with its own template?

I called this class a foundation because it's a wrapper around MDC's MDCSlidingTabIndicatorFoundation.

The reason why it needs to do so much manual DOM manipulation is because we use it both for the tab group and the tab nav bar. The nav bar uses a directive for the links so we don't have control over the template.

Ah, I see. Obviously outside the scope of this PR, but maybe we could encapsulate the behavior in a component and then just use ViewContainerRef to create it and attach it to the element with the directive? Not sure if that's actually feasible, just brainstorming...

I think the boilerplate-wise it would end up being about the same amount of code. Ideally we'd turn the tab link into a component, but that would be a breaking change.

crisbeto · 2019-10-25T18:07:59Z

src/material-experimental/mdc-tabs/ink-bar.ts

+   * to match the width of the content rather than the tab host element.
+   */
+  get fitToContent(): boolean { return this._fitToContent; }
+  set fitToContent(fitToContent: boolean) {


I don't think we need the getter here and the setter could be turned into a function. Since this is a private class we don't need to deal with getters and setters.

I just tried that approach but I'm not sure I feel as comfortable with it. There are two conditions I need to meet:

Let clients see what the current value is

Let clients set a new value, and the foundation reacts to it

To satisfy the first condition, there needs to be a public value fitToContent. However, it would not be clear to clients that in order to change this value, they should not write directly to it, but instead use a setFitToContent function.

If clients didn't need to read the value, I'd agree that a setter would work, but in this case I think the getter/setter pattern is ideal

Kristiyan is saying that this component (ink bar) is not part of the public API, and as such doesn't need a getter/setter (which we only need for Angular inputs) compared to using functions

EDIT: Talked with Jeremy, I did miss that y'all are saying to do "getFitToContent" and "setFitToContent" rather than the ES2015 getter/setter. I'll make the change

OLD REPLY:

Sorry, I see that my explanation may be vague regarding this.

When fitInkBarToContent is provided as an input to the MatTabGroup and MatTabNav, they store the value into the MatInkBarFoundation. This will be the source of truth for it's value (not cached in the tab group or nav). When the value is set, the foundation needs to render again, hence the need for a setter (this can be either a native setter or setFitToContent).

When our users want to access the property fitInkBarToContent, we give them the value stored in MatInkBarFoundation. There are two ways we can do this, expose the property fitToContent, or provide a getFitToContent function. If we expose the property, it's not clear from the MatTabGroup/MatTabNav that it should only use that property for getting, not for setting. In the future, we may make a change and not realize the setter must be used, not the property.

If this doesn't make sense, I may be missing what you are proposing. Let's chat in the next team meeting

Andrew and I chatted, he just didn't follow that we meant do e.g. getFitToContent and setFitToContent instead

crisbeto · 2019-10-25T18:10:24Z

src/material-experimental/mdc-tabs/ink-bar.ts

  private _createIndicator(document: Document) {
+    this._indicator = document.createElement('span');


I think the boilerplate-wise it would end up being about the same amount of code. Ideally we'd turn the tab link into a component, but that would be a breaking change.

jelbourn · 2019-10-25T20:49:18Z

src/material-experimental/mdc-tabs/ink-bar.ts

+   * to match the width of the content rather than the tab host element.
+   */
+  get fitToContent(): boolean { return this._fitToContent; }
+  set fitToContent(fitToContent: boolean) {


Kristiyan is saying that this component (ink bar) is not part of the public API, and as such doesn't need a getter/setter (which we only need for Angular inputs) compared to using functions

jelbourn · 2019-10-25T20:50:06Z

src/material-experimental/mdc-tabs/ink-bar.ts

@@ -128,16 +141,36 @@ export class MatInkBarFoundation {
    this._destroyed = true;
  }

+  /** Creates and appends the indicator element. */


This should also explain what an "indicator" is.

Since indicator and ink bar are synonymous, I renamed our use of the indicator to be consistently called the "ink bar". Alternatively we can rename everything to "indicator" (and also rename classes like MatInkBar to MatTabIndicator).

Where we do mention indicator (in comments referring to MDC's indicator), I made it clearer that we named it ink bar.

jelbourn · 2019-10-25T20:51:11Z

src/material-experimental/mdc-tabs/tab-label-wrapper.ts

+  get fitInkBarToContent(): boolean { return this._foundation.fitToContent; }
+  set fitInkBarToContent(v: boolean) { this._foundation.fitToContent = coerceBooleanProperty(v); }
+
+  constructor(public elementRef: ElementRef, @Inject(DOCUMENT) private _document: any) {


This should define this._document at the class level as type Document, and then set the injected document: any in the constructor

(we use this pattern elsewhere in the library)

…ntent

jelbourn

LGTM

jelbourn · 2019-10-28T21:00:15Z

src/material-experimental/mdc-tabs/ink-bar.ts

+   * to match the width of the content rather than the tab host element.
+   */
+  get fitToContent(): boolean { return this._fitToContent; }
+  set fitToContent(fitToContent: boolean) {


Andrew and I chatted, he just didn't follow that we meant do e.g. getFitToContent and setFitToContent instead

angular-automatic-lock-bot · 2019-11-29T00:04:42Z

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

andrewseguin added P2 The issue is important to a large percentage of users, with a workaround pr: merge safe labels Oct 25, 2019

andrewseguin requested review from jelbourn, crisbeto and mmalerba October 25, 2019 17:13

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 25, 2019

mmalerba reviewed Oct 25, 2019

View reviewed changes

crisbeto reviewed Oct 25, 2019

View reviewed changes

jelbourn reviewed Oct 25, 2019

View reviewed changes

andrewseguin added 2 commits October 28, 2019 13:06

feat(material-experimental/mdc-tabs): add option to fit ink bar to co…

f52f00d

…ntent

rename our use of indicator to ink bar

011831f

andrewseguin force-pushed the mdc-tabs-fit-content branch from cb3cbde to 011831f Compare October 28, 2019 20:28

jelbourn approved these changes Oct 28, 2019

View reviewed changes

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Oct 28, 2019

use getters/setters in ink bar foundation

2b84588

andrewseguin added the target: patch This PR is targeted for the next patch release label Oct 29, 2019

andrewseguin merged commit 77d51ca into angular:master Oct 29, 2019

CharlBest mentioned this pull request Nov 12, 2019

Angular Ivy Upgrade CharlBest/nean-stack-starter#192

Open

4 tasks

angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(material-experimental/mdc-tabs): add option to fit ink bar to content #17507

feat(material-experimental/mdc-tabs): add option to fit ink bar to content #17507

andrewseguin commented Oct 25, 2019

mmalerba Oct 25, 2019

crisbeto Oct 25, 2019 •

edited

Loading

mmalerba Oct 25, 2019

crisbeto Oct 25, 2019

crisbeto Oct 25, 2019

andrewseguin Oct 25, 2019

jelbourn Oct 25, 2019

andrewseguin Oct 28, 2019 •

edited

Loading

jelbourn Oct 28, 2019

crisbeto Oct 25, 2019

jelbourn Oct 25, 2019

jelbourn Oct 25, 2019

andrewseguin Oct 28, 2019

jelbourn Oct 25, 2019

jelbourn left a comment

jelbourn Oct 28, 2019

angular-automatic-lock-bot bot commented Nov 29, 2019

		private _createIndicator(document: Document) {
		this._indicator = document.createElement('span');

feat(material-experimental/mdc-tabs): add option to fit ink bar to content #17507

feat(material-experimental/mdc-tabs): add option to fit ink bar to content #17507

Conversation

andrewseguin commented Oct 25, 2019

Choose a reason for hiding this comment

crisbeto Oct 25, 2019 • 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

andrewseguin Oct 28, 2019 • 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

jelbourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Nov 29, 2019

crisbeto Oct 25, 2019 •

edited

Loading

andrewseguin Oct 28, 2019 •

edited

Loading