fix(toolbar): no longer auto-generate toolbar rows #6661

devversion · 2017-08-27T18:13:57Z

Currently the toolbar always generates the first <md-toolbar-row>. This means that developers have no opportunity to set directives/attributes/classes on the first toolbar row (e.g with flex-layout).

With this change, the toolbar won't auto-generate any <md-toolbar-row> element.

The toolbar will have two different row modes:

Single row toolbar

<md-toolbar>
  First Tow
</md-toolbar>

Multiple rows toolbar

<md-toolbar>
  <md-toolbar-row>First Row</md-toolbar-row>
  <md-toolbar-row>Second Row</md-toolbar-row>
</md-toolbar>

This means that mixing those two row modes is no longer possible and allowed

<md-toolbar>
  <span>First Row</span>
  <md-toolbar-row>Second Row</md-toolbar-row>
</md-toolbar>

BREAKING CHANGE: <md-toolbar-row> elements will be no longer auto-generated for the first row. Meaning that custom styling for the first <md-toolbar-row> is no longer working as before. Since no toolbar-row is auto-generated anymore, there are two different modes for the toolbar now. Either place content directly inside of the <md-toolbar> or place multiple <md-toolbar-row> elements.

Fixes #6004. Fixes #1718.

jelbourn · 2017-08-30T21:21:10Z

Did were ever discuss just not having any md-toolbar-row at all when they're not specified? What was the downside there?

jelbourn · 2017-08-30T21:37:26Z

src/lib/toolbar/toolbar.ts

    super(renderer, elementRef);
  }

+  ngAfterViewInit() {
+    // Do nothing if we're not on the browser platform.
+    if (!this._platform.isBrowser) {


If you abort here on the server, the prerendered toolbar probably won't look right

jelbourn · 2017-08-30T21:40:03Z

src/lib/toolbar/toolbar.ts

+    // If the first toolbar row, which will be auto-generated, does not have any projected content,
+    // then the auto-generated first toolbar row should be removed. This gives developers
+    // full control over the first toolbar row (e.g. for use with flex-layout).
+    if (!firstRowElement.innerHTML.trim()) {


Instead of innerHTML, check !firstRow.textContent.trim() && !firstRow.childNodes.length ?
(I don't even like looking at innerHTML if it can be avoided)

Yeah pretty sure that works too. Let's see if we actually still need that check with the discussed changes on Slack.

willshowell · 2017-08-31T00:13:55Z

What about only projecting md-toolbar-row?

<div class="mat-toolbar-layout">
  <ng-content select="md-toolbar-row, mat-toolbar-row"></ng-content>
</div>

I guess it adds a little fluff to the api, but the behavior would be straightforward and expected.

devversion · 2017-08-31T00:29:25Z

@willshowell I chatted with @jelbourn today about an alternative API design. Should have the PR ready tomorrow.

devversion · 2017-08-31T18:30:49Z

@jelbourn Applied the changes we discussed per Chat. Please have another look.

jelbourn · 2017-09-01T17:56:48Z

src/lib/toolbar/toolbar.md

  </md-toolbar-row>
 </md-toolbar>
 ```

+**Note**: Placing content outside of a `<md-toolbar-row>` when multiple rows are specified, is not
+supported.


Superfluous comma

jelbourn · 2017-09-01T17:58:57Z

src/lib/toolbar/toolbar.ts

@@ -39,15 +42,43 @@ export const _MdToolbarMixinBase = mixinColor(MdToolbarBase);
  inputs: ['color'],
  host: {
    'class': 'mat-toolbar',
-    'role': 'toolbar'
+    'role': 'toolbar',


This role should be removed; I never noticed it being added. It's not appropriate for most uses of the md-toolbar.
(see the docs)

jelbourn · 2017-09-01T18:00:04Z

src/lib/toolbar/toolbar.ts

+export class MdToolbar extends _MdToolbarMixinBase implements CanColor, AfterViewInit {
+
+  /** Reference to all toolbar row elements that have been projected. */
+  @ContentChildren(MdToolbarRow) _toolbarRows: QueryList<MdToolbarRow>;


We need to subscribe to the changes on _toolbarRows and update accordingly, since ngIf and ngFor etc. will affect the content children

Doesn't Angular automatically update the _toolbarRows.length property? I tried it locally and it seemed to work.

It does, but ngAfterViewInit is only run once

That makes sense then. Although it doesn't really handle every case. e.g.

<md-toolbar> <ng-container *ngIf="test">First Row Overwrite</ng-container> <md-toolbar-row>First Row</md-toolbar-row> </md-toolbar>

I'm not sure if we should care about that case as well? It may involve using the ngDoCheck lifecycle hook (which may not be that good for performance)

Yeah, there's not much we can do about that. I think it's fine.

jelbourn

LGTM for code. Can you amend your commit to include a BREAKING CHANGE message? Add merge-ready when ready

jelbourn · 2017-09-06T00:17:54Z

Caretaker note: this is a breaking change. For anyone that has custom styling on md-toolbar-row but isn't explicitly putting md-toolbar-row in their template, we'll need to add it to their template.

devversion · 2017-09-06T10:48:19Z

@jelbourn Done. Added merge-ready, but would be still happy if you briefly look at the breaking change message (I assume for the changelog it will be updated anyway again)

mmalerba · 2017-09-15T21:17:23Z

please rebase

devversion · 2017-09-16T13:05:41Z

@mmalerba Done. e2e screenshot uploading seems to timeout (unrelated)

Currently the toolbar always generates the first `<md-toolbar-row>`. This means that developers have no opportunity to set directives/attributes/classes on the first toolbar row (e.g with flex-layout). With this change, the toolbar won't auto-generate any `<md-toolbar-row>` element. The toolbar will have two different row modes: _Single row toolbar_ ```html <md-toolbar> First Tow </md-toolbar> ``` _Multiple rows toolbar_ ```html <md-toolbar> <md-toolbar-row>First Row</md-toolbar-row> <md-toolbar-row>Second Row</md-toolbar-row> </md-toolbar> ``` This means that mixing those two row modes is no longer possible and allowed ```html <md-toolbar> <span>First Row</span> <md-toolbar-row>Second Row</md-toolbar-row> </md-toolbar> ``` BREAKING CHANGE: `<md-toolbar-row>` elements will be no longer auto-generated for the first row. Meaning that custom styling for the first `<md-toolbar-row>` is no longer working as before. Since no toolbar-row is auto-generated anymore, there are two different modes for the toolbar now. Either place content directly inside of the `<md-toolbar>` or place multiple `<md-toolbar-row>` elements. Fixes angular#6004. Fixes angular#1718.

angular-automatic-lock-bot · 2019-09-07T12:03:35Z

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

devversion added the pr: needs review label Aug 27, 2017

devversion assigned jelbourn Aug 27, 2017

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

jelbourn reviewed Aug 30, 2017

View reviewed changes

devversion force-pushed the fix/toolbar-auto-generated-row branch from 6520e9b to 89e9243 Compare August 31, 2017 18:29

devversion requested a review from amcdnl as a code owner August 31, 2017 18:29

devversion changed the title ~~fix(toolbar): don't auto-generate first row if first row would be empty~~ fix(toolbar): no longer auto-generate toolbar rows Aug 31, 2017

jelbourn reviewed Sep 1, 2017

View reviewed changes

devversion force-pushed the fix/toolbar-auto-generated-row branch from 89e9243 to 9927677 Compare September 1, 2017 18:12

jelbourn approved these changes Sep 6, 2017

View reviewed changes

jelbourn added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note pr: lgtm and removed pr: needs review labels Sep 6, 2017

devversion force-pushed the fix/toolbar-auto-generated-row branch from 8857fbf to 8e8aa4b Compare September 6, 2017 10:47

devversion added the action: merge The PR is ready for merge by the caretaker label Sep 6, 2017

devversion added the P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful label Sep 15, 2017

mmalerba removed the action: merge The PR is ready for merge by the caretaker label Sep 15, 2017

devversion force-pushed the fix/toolbar-auto-generated-row branch from 8e8aa4b to aead062 Compare September 16, 2017 11:24

devversion added the action: merge The PR is ready for merge by the caretaker label Sep 16, 2017

devversion force-pushed the fix/toolbar-auto-generated-row branch 2 times, most recently from af61632 to caee86a Compare September 22, 2017 14:34

devversion force-pushed the fix/toolbar-auto-generated-row branch from caee86a to ccc2608 Compare September 29, 2017 13:14

kara added the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label Oct 5, 2017

devversion force-pushed the fix/toolbar-auto-generated-row branch from ccc2608 to aeb3bd0 Compare October 5, 2017 15:07

andrewseguin merged commit c3405aa into angular:master Oct 31, 2017

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

fix(toolbar): no longer auto-generate toolbar rows #6661

fix(toolbar): no longer auto-generate toolbar rows #6661

Uh oh!

Conversation

devversion commented Aug 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jelbourn commented Aug 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willshowell commented Aug 31, 2017

Uh oh!

devversion commented Aug 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devversion commented Aug 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

jelbourn commented Sep 6, 2017

Uh oh!

devversion commented Sep 6, 2017

Uh oh!

mmalerba commented Sep 15, 2017

Uh oh!

devversion commented Sep 16, 2017

Uh oh!

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

Uh oh!

Uh oh!

devversion commented Aug 27, 2017 •

edited

Loading

devversion commented Aug 31, 2017 •

edited

Loading