refactor(list): simplify list component scss #9194

devversion · 2016-07-31T17:13:07Z

Simplifies the List components SCSS
Fixes Dense Mode for List and introduces super elegant approach
Make SCSS more modular by having a base and dynamic variables
Remove unnecessary DOM node level for button wrap.
No longer wrap proxied elements inside of buttons
Multiple Secondary Items should align properly and have consistent spacing.
Dense Mode should not cut characaters on different environments.

@EladBezalel and I had a short meeting and I showed him some of the new approaches.

Fixes #6152. Fixes #8890 References #8482.

EladBezalel · 2016-07-31T20:05:07Z

src/components/list/list-base.scss

@@ -0,0 +1,300 @@
+/** External component calculations */
+$md-checkbox-width: 3 * $baseline-grid !default;
+//TODO(devversion): use shared variable from buttons.scss


If it's for future work please file an issue and write it's number on the comment

Since it's an issue, which is not depending on that PR, it would make sense to create it.

See #9196

EladBezalel · 2016-07-31T20:12:06Z

I really like the approach we should consider future work on dense components to behave the same.

Sort of related question:
Are we applying dense mode automatically on small screens?

@clshortfuse

devversion · 2016-07-31T20:26:02Z

@EladBezalel - Yeah as discussed in our talk. I'll create a follow-up PR, which makes the dense helpers more generic and usable for other components. I'll create an issue once this PR is available.

EladBezalel · 2016-08-07T13:56:07Z

src/components/list/list-base.scss

@@ -99,6 +99,8 @@ $md-list-item-subheader-height: 6 * $baseline-grid !default;
    margin-top: $baseline-grid;
    margin-bottom: $baseline-grid;
    border-radius: 50%;
+
+    flex-shrink: 0; // Per https://bugs.webkit.org/show_bug.cgi?id=146020


It looks like its resolved already

Status: RESOLVED FIXED

It's resolved in the latest Webkit version, but it's not resolved in the latest Safari version (probably in the Technology Preview).

I tested that on my Mac Book and the bug is still valid.

devversion · 2016-09-19T12:31:57Z

@topherfangio Friendly ping for review if you find some time ;)

topherfangio

Codewise looks pretty good. Found a few issues when running a visual diff (manual..no automation yet):

You can view my diff along with the comments at You can view comments at https://invis.io/2D8TWLO7S

The avatars on the Basic Usage > Normal > 3 line item demo appear to be top-aligned instead of middle-aligned (Screens 1 & 2).
In the same demo, the Dense mode "Classes" section looks better in this PR than in latest, so good work there 👍 (Screens 3 & 4)
The List Controls demo is vastly different regarding spacing/padding (Screens 5 & 6). Is the PR aligned with the spec?

topherfangio · 2016-10-04T19:47:37Z

src/components/list/list.js

@@ -381,7 +380,8 @@ function mdListItemDirective($mdAria, $mdConstant, $mdUtil, $timeout) {
          secondaryItem = buttonWrapper[0];
        }

-        if (secondaryItem && (!hasClickEvent(secondaryItem) || (!tAttrs.ngClick && isProxiedElement(secondaryItem)))) {
+        if (secondaryItem && !tAttrs.ngClick && !hasClickEvent(secondaryItem)


Can you describe why this logic has changed?

This logic was invalid, but not really often causing any issues.

We should not remove the md-secondary class when the element just doesn't have any ng-click on it (-> this means it will be marked as a proxied element)

Now it only targets the element for being a proxied element when all conditions are met .

See issue #6152

src/components/list/list.spec.js

topherfangio · 2016-10-04T19:59:27Z

src/components/list/list.spec.js

+
+    expect($rootScope.isChecked).toBeFalsy();
+
+    var clickListener = listItem[0].querySelector('div');


Can we rename this to something more descriptive about the actual container? listener immediately makes me think this is an event listener rather than a DOM element.

Yeah I agree with that. Just changed and also added a comment :)

jelbourn · 2016-12-02T23:32:20Z

@devversion can you rebase?

* Simplifies the List components SCSS * Fixes Dense Mode for List and introduces super elegant approach * Make SCSS more modular by having a base and dynamic variables * Remove unnecessary DOM node level for button wrap. * No longer wrap proxied elements inside of buttons * Multiple Secondary Items should align properly and have consistent spacing. * Dense Mode should not cut characaters on different environments. Fixes angular#6152. Fixes angular#8890 References angular#8482.

devversion · 2016-12-03T19:26:54Z

@jelbourn Done.

This reverts commit 085c5fd.

Fixes angular#6152. Fixes angular#8890. References angular#8482.

devversion added the needs: review This PR is waiting on review from the team label Jul 31, 2016

devversion assigned ThomasBurleson and EladBezalel Jul 31, 2016

devversion force-pushed the refactor/list-component-scss branch from d80ad21 to 77ff050 Compare July 31, 2016 17:22

devversion mentioned this pull request Jul 31, 2016

fix(list): do not wrap proxied element when using ng-click #9191

Closed

EladBezalel reviewed Jul 31, 2016
View reviewed changes

devversion mentioned this pull request Jul 31, 2016

Build: Access variables from other components #9196

Closed

devversion force-pushed the refactor/list-component-scss branch 3 times, most recently from 9f48da1 to 1a49b9e Compare August 5, 2016 18:23

EladBezalel reviewed Aug 7, 2016
View reviewed changes

devversion force-pushed the refactor/list-component-scss branch from 78eeda9 to 203e2f8 Compare August 8, 2016 19:00

ThomasBurleson added this to the 1.2.0 milestone Aug 22, 2016

devversion force-pushed the refactor/list-component-scss branch from 203e2f8 to fa8529d Compare August 29, 2016 10:14

ThomasBurleson added the needs: group review label Sep 7, 2016

devversion mentioned this pull request Sep 8, 2016

mdListItem. If inner element has "transform" property it's not clickable #9549

Closed

topherfangio added needs: manual testing This issue or PR needs to have some manual testing and verification done and removed needs: group review labels Sep 9, 2016

topherfangio self-assigned this Sep 9, 2016

topherfangio removed the needs: group review label Sep 9, 2016

devversion added the needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved label Sep 10, 2016

devversion force-pushed the refactor/list-component-scss branch from fa8529d to b410e20 Compare September 19, 2016 12:31

devversion removed the needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved label Sep 19, 2016

devversion mentioned this pull request Sep 23, 2016

list: z-index preventing function execution on directive inside md-list-item #9676

Closed

topherfangio suggested changes Oct 4, 2016

View reviewed changes

devversion force-pushed the refactor/list-component-scss branch from b410e20 to 36ce2f0 Compare October 5, 2016 15:23

topherfangio approved these changes Oct 5, 2016

View reviewed changes

topherfangio added needs: presubmit and removed needs: manual testing This issue or PR needs to have some manual testing and verification done needs: review This PR is waiting on review from the team labels Oct 5, 2016

devversion mentioned this pull request Oct 13, 2016

Accessibility Issue with md-list-items #9831

Closed

devversion force-pushed the refactor/list-component-scss branch from 36ce2f0 to 545d706 Compare October 14, 2016 18:53

devversion force-pushed the refactor/list-component-scss branch from 545d706 to c406c34 Compare December 3, 2016 12:55

googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Dec 3, 2016

devversion force-pushed the refactor/list-component-scss branch from c406c34 to a4ff3c5 Compare December 3, 2016 12:56

kara added pr: merge ready This PR is ready for a caretaker to review and removed needs: presubmit labels Jan 4, 2017

kara merged commit 085c5fd into angular:master Jan 4, 2017

devversion deleted the refactor/list-component-scss branch January 4, 2017 17:21

devversion added a commit to devversion/material that referenced this pull request Jan 4, 2017

Revert "refactor(list): simplify list component scss (angular#9194)"

b7fb136

This reverts commit 085c5fd.

kara mentioned this pull request Jan 4, 2017

Revert "refactor(list): simplify list component scss" #10221

Merged

ThomasBurleson modified the milestones: 1.1.3, 1.2.0 Jan 4, 2017

davidenochk pushed a commit to davidenochk/material that referenced this pull request Feb 3, 2017

refactor(list): simplify list component scss (angular#9194)

c55f7d0

Fixes angular#6152. Fixes angular#8890. References angular#8482.

Splaktar modified the milestones: 1.1.3, 1.1.2 Mar 1, 2018

Splaktar unassigned topherfangio Mar 1, 2018

Splaktar mentioned this pull request Dec 9, 2018

list: restructure the component #8482

Open

Splaktar removed this from the 1.1.2 milestone Sep 23, 2019

Splaktar unassigned ThomasBurleson Sep 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(list): simplify list component scss #9194

refactor(list): simplify list component scss #9194

devversion commented Jul 31, 2016

EladBezalel Jul 31, 2016

devversion Jul 31, 2016

EladBezalel commented Jul 31, 2016

devversion commented Jul 31, 2016 •

edited

Loading

EladBezalel Aug 7, 2016 •

edited

Loading

devversion Aug 8, 2016 •

edited

Loading

devversion commented Sep 19, 2016

topherfangio left a comment

topherfangio Oct 4, 2016

devversion Oct 5, 2016

topherfangio Oct 4, 2016

devversion Oct 5, 2016

jelbourn commented Dec 2, 2016

devversion commented Dec 3, 2016


		expect($rootScope.isChecked).toBeFalsy();

		var clickListener = listItem[0].querySelector('div');

refactor(list): simplify list component scss #9194

refactor(list): simplify list component scss #9194

Conversation

devversion commented Jul 31, 2016

EladBezalel Jul 31, 2016

Choose a reason for hiding this comment

devversion Jul 31, 2016

Choose a reason for hiding this comment

EladBezalel commented Jul 31, 2016

devversion commented Jul 31, 2016 • edited Loading

EladBezalel Aug 7, 2016 • edited Loading

Choose a reason for hiding this comment

devversion Aug 8, 2016 • edited Loading

Choose a reason for hiding this comment

devversion commented Sep 19, 2016

topherfangio left a comment

Choose a reason for hiding this comment

topherfangio Oct 4, 2016

Choose a reason for hiding this comment

devversion Oct 5, 2016

Choose a reason for hiding this comment

topherfangio Oct 4, 2016

Choose a reason for hiding this comment

devversion Oct 5, 2016

Choose a reason for hiding this comment

jelbourn commented Dec 2, 2016

devversion commented Dec 3, 2016

devversion commented Jul 31, 2016 •

edited

Loading

EladBezalel Aug 7, 2016 •

edited

Loading

devversion Aug 8, 2016 •

edited

Loading