feat(divider): move divider out of mat-list #5862

CaerusKaru · 2017-07-19T04:47:56Z

Moves mat-divider out of MatListModule, replaced with temporary import to prevent breaking changes. Also includes support for inset and vertical dividers.

Fixes #5524.
Fixes #2017.

Gustav0ar · 2017-07-21T13:58:42Z

@CaerusKaru in divider.scss you need to add:
&.mat-divider-vertical {
border-right-color: rgba(0, 0, 0, 0.12);
}

Otherwise the color will be black and not following the standard

CaerusKaru · 2017-07-21T14:05:00Z

@Gustav0ar the color is set in _divider-theme.scss

Gustav0ar · 2017-07-21T14:06:23Z

Ah, ok, my bad. I missed reviewing that file

rafaelss95 · 2017-10-12T00:28:37Z

src/demo-app/list/list-demo.html

@@ -22,6 +22,7 @@ <h3 mat-line>{{contact.name}}</h3>
      </mat-list-item>
    </mat-list>

+<<<<<<< HEAD


Did you forgot to remove this? Also in line 43.

Resolved, thanks for the catch. I rebased and for some reason Git missed this.

crisbeto · 2017-10-21T13:08:20Z

@CaerusKaru if you rebase from master the errors should be resolved.

CaerusKaru · 2017-10-21T17:45:47Z

Thanks @crisbeto, rebased and ready for review.

ghost

I found two nits.
And what about adding a example for the matMenu to the demo app? Just for the sake of completeness.

ghost · 2017-10-25T14:16:39Z

src/demo-app/card/card-demo.html

+    <mat-divider [inset]="true"></mat-divider>
+    <mat-card-actions>
+      <button md-button>LIKE</button>
+      <button md-button>SHARE</button>


These two should be mat-button.

ghost · 2017-10-25T14:29:41Z

src/demo-app/list/list-demo.html

@@ -31,6 +31,7 @@ <h4 mat-line>{{message.from}}</h4>
          <span>{{message.subject}} -- </span>
          <span class="demo-secondary-text">{{message.message}}</span>
        </p>
+        <mat-divider [inset]="true" *ngIf="!last"></mat-divider>


You forgot to bind last to a local variable: *ngFor="let message of messages; last as last"

Added, this got cut from a previous commit when rebased, good catch

willshowell · 2017-11-09T20:06:58Z

src/lib/divider/divider.md

+| Name            | Type    | Values      | Description                               |
+| --------------- | ------- | ----------- | ----------------------------------------- |
+| inset           | boolean | true, false | Whether the divider is an inset divider   |
+| vertical        | boolean | true, false | Whether the divider is a vertical divider |


IMO the values column is covered by them both having boolean type. Further, I'm not sure this table is even necessary, since it will be available on the api page.

Originally pulled from list, but now I see it's been removed. Will fix.

willshowell · 2017-11-09T20:15:23Z

src/lib/divider/divider.md

+`<mat-divider>` is a container component that wraps and formats a series of line items. As the base
+list component, it provides Material Design styling, but no behavior of its own.
+
+<!-- example(divider-overview) -->


There doesn't seem to be a divider example?

Do you mean in the material-examples directory?

I see now that the prose is wrong here, will correct.

Yep. It would probably be good to add one in with this PR anyway.

josephperrott · 2017-11-09T22:39:16Z

src/lib/divider/divider.spec.ts

+  });
+});
+
+@Component({


Rather than creating a number of Components that have to be recompiled on every run, can you create a single Component that is altered through attributes for tests?

We have found a bottleneck in our tests comes from having to recompile many components on each test run.

@Component({ template: `<mat-divider [inset]="inset" [vertical]="vertical"></mat-divider>` }) class MatDividerTestComponent { vertical: boolean; inset: boolean; }

Then in tests you would do:

it('should apply a vertical class', () => { fixture.componentInstance.vertical = true; fixture.detechChanges(); expect(divider.nativeElement.className).toContain('mat-divider-vertical'); });

Great suggestions, added

josephperrott · 2017-11-09T22:41:12Z

src/lib/divider/divider.spec.ts

+  }));
+
+  it('should not apply any additional class to a list without lines', () => {
+    const fixture = TestBed.createComponent(MatDividerBase);


Instead of calling TestBed.createComponent in every test, set up a beforeEach block to do this setup.

let fixture: ComponentFixture;

beforeEach(() => {
fixture = TestBed.createComponent(MatDividerTestComponent);
});

josephperrott · 2017-11-09T22:42:35Z

src/lib/divider/divider.ts

+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import {


Please put one import per line if it doesn't fit on a single line:

import { ChangeDetectionStrategy, Component, Input, ViewEncapsulation } from '@angular/core';

josephperrott · 2017-11-09T22:44:43Z

src/lib/divider/divider.scss

+
+.mat-list-item {
+  .mat-divider {
+    position: absolute;


There seems to be inconsistent spacing between attributes, can you make the spacing more consistent?

josephperrott · 2017-11-09T22:46:33Z

src/lib/divider/divider.scss

+    border-top: 0;
+    border-right-width: 1px;
+    border-right-style: solid;
+    position: static;


position: static and width: auto should not be necessary as these are the default values on any custom element.

josephperrott · 2017-11-09T22:48:02Z

src/lib/divider/divider.scss

+  }
+
+  &.mat-divider-inset {
+    margin-left: 80px;


Where does this 80px size come from? Can you put in a comment explaining it?

It came from AngularJS Material found here. baseline-grid = 8.

I don't really have an explanation for it, I couldn't find one in the original repo. Is there any way you can reach out to the internal UX team to find verification/justification?

josephperrott · 2017-11-09T22:48:45Z

src/lib/divider/divider.scss

+  }
+
+  &.mat-divider-inset {
+    margin-left: 80px;


For the inset's margin size, can you move the value (80px) into a variable at the top of the file?

josephperrott · 2017-11-09T22:49:20Z

src/lib/divider/divider.scss

+.mat-divider {
+  display: block;
+  margin: 0;
+  border-top-width: 1px;


For the dividers line width (based on the border width), can you move the value (1px) into a variable at the top of the file?

josephperrott · 2017-11-09T22:50:06Z

src/lib/divider/divider.scss

+  }
+}
+
+.mat-list-item {


Rules specific to other components should be in the the respective component since it is an override specifically for the component.

josephperrott · 2017-11-09T22:51:29Z

src/lib/divider/divider.ts

+export class MatDivider {
+  /** Whether the divider is vertically aligned. */
+  @Input() get vertical(): boolean { return this._vertical; }
+  set vertical(value: boolean) {


for both setter methods, you should be able to move them to a single line which might be a bit more readable.

julianobrasil · 2017-11-10T09:28:30Z

src/lib/divider/divider.md

+Dividers can be added to lists as a means of separating content into distinct sections.
+Inset dividers can also be added to provide the appearance of distinct elements in a list without cluttering content
+like avatar images or icons. If combining both, please make sure to avoid adding an inset divider to the last element
+in a list, because it will overlap with the section divider.


What about:

~~If combining both, please m~~Make sure to avoid adding an inset divider to the last element in a list, because it will overlap with the section divider

josephperrott · 2017-12-08T22:25:48Z

src/lib/card/card.scss

@@ -16,6 +16,20 @@ $mat-card-header-size: 40px !default;
  padding: $mat-card-default-padding;
  border-radius: $mat-card-border-radius;

+  .mat-divider {


can you separate the attributes from the rules here?

.mat-divider { position: absolute; left: 0; width: 100%; [dir='rtl'] & { left: auto; right: 0; } &.mat-divider-inset { position: static; margin: 0; } }

josephperrott · 2017-12-08T22:29:30Z

src/lib/list/list.scss

@@ -116,6 +118,26 @@ $mat-dense-list-icon-size: 20px;
    border-radius: 50%;
    padding: 4px;
  }
+
+  .mat-divider {


can you separate the attributes from the rules here?

.mat-divider { position: absolute; bottom: 0; left: 0; width: 100%; [dir='rtl'] & { left: auto; right: 0; } &.mat-divider-inset { left: $mat-list-item-inset-divider-offset; width: calc(100% - #{$mat-list-item-inset-divider-offset}); margin: 0; [dir='rtl'] & { left: auto; right: 72px; } } }

- Creates an independent mat-divider component - Adds inset capability with special accomodations for mat-list-item - Adds vertical option - Removed mat-divider from MatListModule with temporary import to prevent breaking changes - Added styling for dividers in cards - Add demos for use in list, card, and menu

josephperrott

LGTM

josephperrott · 2017-12-13T23:46:14Z

Caretaker Note: This change should be entirely transparent to end users, but we should take careful note on it to be sure that nothing changes as it is swapping out internal component usage.

angular-automatic-lock-bot · 2019-09-08T00:01:49Z

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 Jul 19, 2017

CaerusKaru force-pushed the divider branch 3 times, most recently from e7d1a42 to 7c93bf2 Compare July 19, 2017 07:07

CaerusKaru requested review from crisbeto and jelbourn as code owners October 11, 2017 23:58

CaerusKaru force-pushed the divider branch from a9acff0 to 70c9d4d Compare October 12, 2017 00:24

rafaelss95 reviewed Oct 12, 2017

View reviewed changes

CaerusKaru force-pushed the divider branch from 70c9d4d to b7b8e34 Compare October 12, 2017 00:36

CaerusKaru changed the title ~~feat(divider): move divider out of md-list~~ feat(divider): move divider out of mat-list Oct 12, 2017

CaerusKaru force-pushed the divider branch 2 times, most recently from 917ce22 to 7da32ed Compare October 12, 2017 04:39

CaerusKaru requested a review from amcdnl as a code owner October 12, 2017 04:39

CaerusKaru force-pushed the divider branch 4 times, most recently from 01aea89 to 06dd188 Compare October 12, 2017 05:50

CaerusKaru force-pushed the divider branch 2 times, most recently from b8ed704 to 8647d64 Compare October 21, 2017 00:33

CaerusKaru requested a review from devversion as a code owner October 21, 2017 00:33

CaerusKaru force-pushed the divider branch from 8647d64 to 5b21a9a Compare October 21, 2017 00:45

CaerusKaru force-pushed the divider branch from 5b21a9a to 0053a2c Compare October 21, 2017 17:20

ghost suggested changes Oct 25, 2017

View reviewed changes

CaerusKaru force-pushed the divider branch from 0053a2c to d32fe60 Compare October 26, 2017 00:52

CaerusKaru requested a review from kara as a code owner October 26, 2017 00:52

CaerusKaru force-pushed the divider branch from 29ddec1 to dddd4cd Compare November 9, 2017 20:06

willshowell reviewed Nov 9, 2017

View reviewed changes

CaerusKaru force-pushed the divider branch 3 times, most recently from f5985ae to d55f5d4 Compare November 9, 2017 20:45

josephperrott reviewed Nov 9, 2017

View reviewed changes

CaerusKaru force-pushed the divider branch 3 times, most recently from bc1c86b to 9ee8778 Compare November 10, 2017 01:31

julianobrasil reviewed Nov 10, 2017

View reviewed changes

CaerusKaru force-pushed the divider branch from 9ee8778 to 3167829 Compare November 10, 2017 09:39

CaerusKaru force-pushed the divider branch 2 times, most recently from 6da04fa to eeec51f Compare November 21, 2017 21:22

CaerusKaru force-pushed the divider branch from eeec51f to 53086f6 Compare December 5, 2017 22:35

josephperrott reviewed Dec 8, 2017

View reviewed changes

CaerusKaru force-pushed the divider branch from 53086f6 to f00650c Compare December 11, 2017 18:12

josephperrott approved these changes Dec 13, 2017

View reviewed changes

josephperrott 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 action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Dec 13, 2017

CaerusKaru force-pushed the divider branch from f00650c to 661dad2 Compare December 16, 2017 20:39

josephperrott merged commit bd0ec64 into angular:master Dec 21, 2017

CaerusKaru deleted the divider branch December 21, 2017 19:38

jotatoledo mentioned this pull request Jan 4, 2018

bug(mat-divider): wrong styling #9233

Closed

ghost mentioned this pull request Jan 26, 2018

[Stackblitz] Add divider module to imports #9618

Closed

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

feat(divider): move divider out of mat-list #5862

feat(divider): move divider out of mat-list #5862

Conversation

CaerusKaru commented Jul 19, 2017 • edited Loading

Gustav0ar commented Jul 21, 2017 • edited Loading

CaerusKaru commented Jul 21, 2017

Gustav0ar commented Jul 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto commented Oct 21, 2017

CaerusKaru commented Oct 21, 2017

ghost left a comment • edited by ghost 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

CaerusKaru Nov 10, 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

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

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

josephperrott left a comment

Choose a reason for hiding this comment

josephperrott commented Dec 13, 2017

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

CaerusKaru commented Jul 19, 2017 •

edited

Loading

Gustav0ar commented Jul 21, 2017 •

edited

Loading

ghost left a comment •

edited by ghost

Loading

CaerusKaru Nov 10, 2017 •

edited

Loading