fix(divider): add inset styles for icons and lists in cards #9242

CaerusKaru · 2018-01-05T05:50:59Z

Add mixin to dynamically apply inset divider offset based on
the precursor in the list item
Add inset divider example using icons as precursor

CaerusKaru · 2018-01-05T06:39:21Z

Circle CI build error is unrelated to this PR

crisbeto · 2018-01-05T11:33:01Z

src/lib/list/list.scss

+// size of the parent class (e.g. avatar vs icon)
+@mixin mat-inset-divider-offset($offset) {
+  $mat-list-item-inset-divider-offset: #{(2 * $mat-list-side-padding) + $offset};
+  left: $mat-list-item-inset-divider-offset;


Why is this one using left/right, whereas the default divider styles are using margin-left/margin-right?

Changed, the reason was that it was legacy from AngularJS Material

crisbeto · 2018-01-05T11:33:48Z

src/lib/list/list.scss

-$mat-list-item-inset-divider-offset: 72px;
+// This mixin provides the correct offset for an inset divider based on the
+// size of the parent class (e.g. avatar vs icon)
+@mixin mat-inset-divider-offset($offset) {


The mixin should live somewhere in the divider module since it could be used in other modules that consume it.

crisbeto · 2018-01-05T11:34:42Z

src/lib/list/list.scss

@@ -27,7 +28,18 @@ $mat-dense-three-line-height: 76px;
 $mat-dense-multi-line-padding: 16px;
 $mat-dense-list-icon-size: 20px;

-$mat-list-item-inset-divider-offset: 72px;


Removing this is a breaking change, even though it isn't being used anywhere.

Since mat-divider hasn't been merged into the minor version yet, can we still remove it? Happy to keep it as a placeholder otherwise

It could still be used from when the divider was in the list module.

crisbeto · 2018-01-05T11:36:22Z

src/lib/list/list.scss

-        left: auto;
-        right: $mat-list-item-inset-divider-offset;
-      }
+      position: absolute;


The position: absolute is defined in the parent rule above, you don't have to add it to .mat-divider-inset. Also the nesting here isn't necessary.

It was present to override a similar position: static in the mat-card styles. I added an !important tag instead with a note for why it's there. I can add the nested structure back if you prefer to not have !important

In that case it can at least use a comment about why the nesting was necessary.

Perfect, will add

crisbeto · 2018-01-06T13:55:27Z

src/lib/list/list.scss

@@ -27,7 +28,18 @@ $mat-dense-three-line-height: 76px;
 $mat-dense-multi-line-padding: 16px;
 $mat-dense-list-icon-size: 20px;

-$mat-list-item-inset-divider-offset: 72px;


It could still be used from when the divider was in the list module.

crisbeto · 2018-01-06T13:56:34Z

src/lib/list/list.scss

  }

  .mat-divider {
-    position: absolute;
+    position: absolute !important; // this is done to override card inset styles


Rather than using !important, perhaps the selector can be made more specific or the card one can be less specific?

The original nesting was the way to make it more specific. I don't think that modifying the card styles will be as elegant. I'll add back the nesting for now unless you have an alternative suggestion

CaerusKaru · 2018-01-06T22:46:12Z

@crisbeto I'm not entirely sure why the Bazel build is failing on Circle CI (I know it's because it doesn't like how I'm including the divider style file). Do you know what's going on?

crisbeto · 2018-01-08T21:06:45Z

I'm not too sure about that error @CaerusKaru, my guess is that you'd need to add another item to the deps array.

CaerusKaru · 2018-01-08T21:14:11Z

@crisbeto I tried adding the divider scss rule to the deps array, but that's what Bazel is complaining about. @jelbourn Any thoughts?

CaerusKaru · 2018-01-08T22:18:42Z

@crisbeto I fixed the Bazel issues and rebased with master. Ready for review again.

crisbeto

LGTM

* Add mixin to dynamically apply inset divider offset based on the precursor in the list item * Add inset divider example using icons as precursor

jelbourn

LGTM

angular-automatic-lock-bot · 2019-09-08T06:02:07Z

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

CaerusKaru requested review from crisbeto, devversion and jelbourn as code owners January 5, 2018 05:51

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

CaerusKaru mentioned this pull request Jan 5, 2018

bug(mat-divider): wrong styling #9233

Closed

CaerusKaru force-pushed the div-fix branch from 0b02849 to 0ca0bed Compare January 5, 2018 06:12

crisbeto requested changes Jan 5, 2018

View reviewed changes

CaerusKaru force-pushed the div-fix branch from 0ca0bed to f9e6be6 Compare January 5, 2018 17:30

crisbeto reviewed Jan 6, 2018

View reviewed changes

CaerusKaru force-pushed the div-fix branch 8 times, most recently from b6536fc to 2f35ebe Compare January 6, 2018 21:51

CaerusKaru force-pushed the div-fix branch 5 times, most recently from 1401968 to b864c92 Compare January 8, 2018 21:50

crisbeto approved these changes Jan 9, 2018

View reviewed changes

crisbeto added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Jan 9, 2018

crisbeto added the target: patch This PR is targeted for the next patch release label Jan 9, 2018

fix(divider): add inset styles for icons and lists in cards

1e8af78

* Add mixin to dynamically apply inset divider offset based on the precursor in the list item * Add inset divider example using icons as precursor

jelbourn approved these changes Jan 10, 2018

View reviewed changes

CaerusKaru force-pushed the div-fix branch from b864c92 to 1e8af78 Compare January 17, 2018 21:02

tinayuangao merged commit d587abe into angular:master Jan 31, 2018

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(divider): add inset styles for icons and lists in cards #9242

fix(divider): add inset styles for icons and lists in cards #9242

CaerusKaru commented Jan 5, 2018

CaerusKaru commented Jan 5, 2018

crisbeto Jan 5, 2018

CaerusKaru Jan 5, 2018

crisbeto Jan 5, 2018

CaerusKaru Jan 5, 2018

crisbeto Jan 5, 2018

CaerusKaru Jan 5, 2018

crisbeto Jan 6, 2018

crisbeto Jan 5, 2018

CaerusKaru Jan 5, 2018

crisbeto Jan 6, 2018

CaerusKaru Jan 6, 2018

crisbeto Jan 6, 2018

crisbeto Jan 6, 2018

CaerusKaru Jan 6, 2018

CaerusKaru commented Jan 6, 2018

crisbeto commented Jan 8, 2018

CaerusKaru commented Jan 8, 2018

CaerusKaru commented Jan 8, 2018

crisbeto left a comment

jelbourn left a comment

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

fix(divider): add inset styles for icons and lists in cards #9242

fix(divider): add inset styles for icons and lists in cards #9242

Conversation

CaerusKaru commented Jan 5, 2018

CaerusKaru commented Jan 5, 2018

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

CaerusKaru commented Jan 6, 2018

crisbeto commented Jan 8, 2018

CaerusKaru commented Jan 8, 2018

CaerusKaru commented Jan 8, 2018

crisbeto left a comment

Choose a reason for hiding this comment

jelbourn left a comment

Choose a reason for hiding this comment

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