feat(list): add new '<mat-action-list>' #12415

vivian-hu-zz · 2018-07-27T22:42:56Z

simple list only in this check in

jelbourn · 2018-07-27T22:46:48Z

.gitignore

@@ -41,4 +42,4 @@ testem.log

 # schematics
 /src/lib/schematics/**/*.js
-/src/lib/schematics/**/*.map
+/src/lib/schematics/**/*.map


Can you configure your IDE to always add a newline at the end of files? Looks like it removed the newline here.

jelbourn · 2018-07-27T22:47:08Z

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

@@ -104,6 +104,15 @@ <h2>Nav lists</h2>
    </mat-nav-list>
  </div>

+  <div>
+    <h2>Button lists</h2>


This should be "Action list"

jelbourn · 2018-07-27T22:47:29Z

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

+    <h2>Button lists</h2>
+    <mat-button-list>
+      <button mat-list-item  *ngFor="let link of links" (click)="onButtonClick(link.name)">
+        {{ link.name }}


Omit space in braces

{{link.name}}

jelbourn · 2018-07-27T22:51:08Z

src/demo-app/list/list-demo.ts

@@ -69,4 +69,8 @@ export class ListDemo {
    this.selectedOptions = values;
    this.modelChangeEventCount++;
  }
+
+  onButtonClick(msg: string) {


Prefer to name methods for what they do rather than how they're called, e.g. alertItem(...)

(this doesn't really matter for demos, but I mention it just as an FYI)

jelbourn · 2018-07-27T22:52:44Z

src/lib/list/list.ts

+  encapsulation: ViewEncapsulation.None,
+  changeDetection: ChangeDetectionStrategy.OnPush,
+})
+export class MatButtonList extends _MatListMixinBase implements CanDisableRipple {}


Since it looks like .mat-button-list doesn't differ from .mat-list for any styles or behavior, you should be able to just add mat-action-list to the existing selector for MatList without creating a new component

jelbourn · 2018-07-27T22:54:35Z

src/lib/list/list.ts

+  selector: 'mat-button-list',
+  exportAs: 'matButtonList',
+  host: {
+    'role': 'button list',


role corresponds to single value that defines the interaction pattern for an element:
https://www.w3.org/WAI/PF/HTML/wiki/RoleAttribute

There isn't any role that corresponds to a list of actions, so this isn't necessary here.

jelbourn · 2018-07-27T22:55:18Z

src/lib/list/list.ts

+
+@Component({
+  moduleId: module.id,
+  selector: 'mat-button-list',


The selector for the new variant should be mat-action-list

jelbourn · 2018-07-27T22:57:39Z

src/lib/list/list.scss

@@ -262,6 +262,22 @@ $mat-list-item-inset-divider-offset: 72px;
  }
 }

+.mat-button-list {
+  //remove the native button look and make is look like a list item
+  button {


Rather than using two levels of selector (.mat-button-list button), it would be slightly better to use a single css class (.mat-action-list-item) for this style. This will have a lower specificity and make it easier for users to customize the css.

We discussed and decided to go with the current nav list pattern.

jelbourn · 2018-07-27T22:58:26Z

src/lib/list/list.ts

@@ -92,7 +109,7 @@ export class MatListSubheaderCssMatStyler {}
 /** An item within a Material Design list. */
 @Component({
  moduleId: module.id,
-  selector: 'mat-list-item, a[mat-list-item]',
+  selector: 'mat-list-item, a[mat-list-item], button[mat-list-item]',


We'll need to add documentation for the new list variant to list.md

crisbeto · 2018-07-28T07:23:32Z

src/lib/list/_list-theme.scss

@@ -27,7 +27,7 @@
  }

  .mat-list-option,
-  .mat-nav-list .mat-list-item {
+  .mat-nav-list .mat-button-list .mat-list-item {


This selector doesn't look correct. It'll target items inside a button list that is inside a nav list.

crisbeto · 2018-07-28T07:25:52Z

src/lib/list/_list-theme.scss

@@ -46,7 +46,7 @@
  }

  // Default list
-  .mat-list, .mat-nav-list, .mat-selection-list {
+  .mat-list, .mat-nav-list, .mat-button-list, .mat-selection-list {


Not something necessarily for this PR, but at some point we should add a class like mat-list-base and use it for cases like this one. Since this selector has a few nested under it, the amount of CSS is going to increase a lot as we add more list types.

Agreed, we should do that in a follow-up PR.

vivian-hu-zz

Please take a look again.

EdricChan03 · 2018-07-31T10:33:25Z

src/lib/list/list.spec.ts

@@ -144,6 +145,14 @@ describe('MatList', () => {
    items.forEach(item => expect(item._isRippleDisabled()).toBe(true));
  });

+  it('action list should be created successfully', () => {


Looks like the grammar wouldn't make sense here when testing with Karma.

This would make more sense:

it('should create an action list', () => {})

EdricChan03 · 2018-07-31T10:33:45Z

src/lib/list/list.spec.ts

+@Component({template: `
+  <mat-action-list>
+    <button mat-list-item>
+        Paprika


Indenting here should be 2 spaces instead of 4

EdricChan03 · 2018-07-31T10:34:05Z

.gitignore

@@ -27,6 +27,7 @@ node_modules
 # IDEs
 /.idea
 /.vscode
+*.iml


Where did this additional line come from?

This is the ide file

EdricChan03 · 2018-07-31T10:34:47Z

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

+  <div>
+    <h2>Action list</h2>
+    <mat-action-list>
+      <button mat-list-item  *ngFor="let link of links" (click)="alertItem(link.name)">


Extra whitespace character after mat-list-item

EdricChan03 · 2018-07-31T10:35:28Z

src/demo-app/list/list-demo.ts

@@ -69,4 +69,8 @@ export class ListDemo {
    this.selectedOptions = values;
    this.modelChangeEventCount++;
  }
+
+  alertItem(msg: string) {
+    alert(msg);


Maybe use a snack bar instead?

A snack bar will look nicer. It can be done later in my opinion. I am new to angular, so try to do small check in without taking too long to deal with merge conflict etc.

This app is only for our own development, so it doesn't really matter what the action does.

EdricChan03 · 2018-07-31T10:36:31Z

src/lib/list/list.md

@@ -42,6 +42,20 @@ element in an `<mat-list-item>`.
 </mat-nav-list>
 ```

+### Action lists
+
+Use `mat-action-list` tags for action lists (i.e. lists that have button tags).


Maybe rephrase this sentence to this?

A <mat-action-list> element is used for action lists (i.e. lists that have button tags.)

(This is based on the earlier section about navigation lists.)

The navigation list is actually doing the same as what I have put in originally. And I feel "use xxx" is more clear than "xxx is used" here.

I would word this as

Use the `<mat-action-list>` element when each item in the list performs some _action_. Each item in an action list is a `<button>` element. 

I think it would also be good to change the nav-list section similarly, but that can be done in another PR.

EdricChan03 · 2018-07-31T10:36:48Z

src/lib/list/list.md

+
+```html
+<mat-action-list>
+  <button mat-list-item  *ngFor="let link of links" (click)="alertItem(link.name)">


Extra whitespace character after mat-list-item

Show what the alertItem function does

removed the white space. Also removed the click part since it is not showing the key point here and may cause unnecessary confusion.

EdricChan03 · 2018-07-31T10:37:41Z

src/lib/list/list.scss

@@ -262,6 +262,22 @@ $mat-list-item-inset-divider-offset: 72px;
  }
 }

+mat-action-list {
+  //remove the native button look and make is look like a list item


Nitpick: is should be it

ngbot · 2018-07-31T16:27:57Z

Hi @vivian-hu! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

jelbourn · 2018-07-31T18:12:17Z

src/lib/list/list.md

@@ -42,6 +42,20 @@ element in an `<mat-list-item>`.
 </mat-nav-list>
 ```

+### Action lists
+
+Use `mat-action-list` tags for action lists (i.e. lists that have button tags).


I would word this as

Use the `<mat-action-list>` element when each item in the list performs some _action_. Each item in an action list is a `<button>` element. 

I think it would also be good to change the nav-list section similarly, but that can be done in another PR.

jelbourn · 2018-07-31T22:32:17Z

src/lib/list/list.md

+```html
+<mat-action-list>
+  <button mat-list-item *ngFor="let link of links"> {{link.name}} </button>
+</mat-action-list>


I would make the following changes:

<mat-action-list> <button mat-list-item (click)="save()">Save</button> <button mat-list-item (click)="undo()">Undo</button> </mat-action-list>

Getting rid of ngFor makes it a bit simpler

Adding a (click) handler shows how the action is performed

jelbourn · 2018-07-31T22:35:39Z

src/lib/list/list.ts

@@ -92,7 +92,7 @@ export class MatListSubheaderCssMatStyler {}
 /** An item within a Material Design list. */
 @Component({
  moduleId: module.id,
-  selector: 'mat-list-item, a[mat-list-item]',
+  selector: 'mat-list-item, a[mat-list-item], button[mat-list-item]',


One last thing that occurred to me just now- we should default all buttons with mat-list-item to have type="button". By default, <button> elements are type="submit", which you would never really want in an action list.

vivian-hu-zz

add default type to button and also added unit testing to test it. Updated wording according to Jeremy's suggestion.

jelbourn · 2018-08-02T18:53:00Z

src/lib/list/list.spec.ts

@@ -144,6 +146,30 @@ describe('MatList', () => {
    items.forEach(item => expect(item._isRippleDisabled()).toBe(true));
  });

+  it('should create an action list', () => {
+    let fixture = TestBed.createComponent(ActionListWithoutType);


Prefer const over let whenever the variable isn't going to be assigned to something else

vivian-hu-zz

fixed const vs. let.

jelbourn

LGTM

jelbourn · 2018-08-07T22:34:32Z

Commit message should be

feat(list): add new `<mat-action-list>`

An action-list is a list where each list item is a native `<button>` element,
meant to be used in cases where each list item has a single action with
no sub-actions, navigation, or selection.

josephperrott · 2018-08-09T17:19:36Z

src/lib/list/list.scss

@@ -262,6 +262,22 @@ $mat-list-item-inset-divider-offset: 72px;
  }
 }

+mat-action-list {
+  //remove the native button look and make it look like a list item
+  button {


Shouldn't this apply to all buttons for any .mat-list-item not just the ones under a mat-action-list?

As we talked, we added mat-action-list specifically for action list to make it more clear, so the idea is to let user use it under mat-action-list.

josephperrott · 2018-08-09T17:20:37Z

src/lib/list/list.ts

+
+    // If no type attributed is specified for <button>, set it to "button".
+    // If a type attribute is already specified, do nothing.
+    let element: HTMLElement = this._getHostElement();


const should be used here instead of let since element does not get reassigned.

josephperrott · 2018-08-09T17:24:57Z

src/lib/list/list.ts

+
+    // If no type attributed is specified for <button>, set it to "button".
+    // If a type attribute is already specified, do nothing.
+    let element: HTMLElement = this._getHostElement();


Since _getHostElement() defines the return type of HTMLElement, you don't need to define the type for the element variable.

josephperrott · 2018-08-09T17:32:40Z

src/lib/list/list.ts

+    // If no type attributed is specified for <button>, set it to "button".
+    // If a type attribute is already specified, do nothing.
+    let element: HTMLElement = this._getHostElement();
+    if (element.nodeName && element.nodeName.toLowerCase() === 'button'


You could actually check tagName. Since element is already a type of HtmlElement, you shouldn't need to check if a tagName is defined since it always will have tagName by definition of the HtmlElement interface.

Additionally, you can just set the attribute type without performing a check beforehand.

if (element.tagName === 'BUTTON') { element.setAttribute('type', element.getAttribute('type') || 'button'); }

I actually prefer always doing nodeName since it's guaranteed to be safe. It should probably be in the coding standards...

vivian-hu-zz

more update based on Joey's feedback

vivian-hu-zz · 2018-08-10T16:51:18Z

src/lib/list/list.scss

@@ -262,6 +262,22 @@ $mat-list-item-inset-divider-offset: 72px;
  }
 }

+mat-action-list {
+  //remove the native button look and make it look like a list item
+  button {


As we talked, we added mat-action-list specifically for action list to make it more clear, so the idea is to let user use it under mat-action-list.

ngbot · 2018-08-31T15:30:47Z

Hi @vivian-hu! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

simple list only in this check in

An action-list is a list where each list item is a native '<button>' element, meant to be used in cases where each list item has a single action with no sub-action, navigation or selection.

angular-automatic-lock-bot · 2019-09-10T10:03:06Z

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

vivian-hu-zz requested review from crisbeto, devversion and jelbourn as code owners July 27, 2018 22:42

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

jelbourn added the target: major This PR is targeted for the next major release label Jul 27, 2018

jelbourn reviewed Jul 27, 2018

View reviewed changes

crisbeto reviewed Jul 28, 2018

View reviewed changes

vivian-hu-zz force-pushed the firstbug branch 2 times, most recently from 224eb2d to 0f5d2ee Compare July 30, 2018 23:26

vivian-hu-zz commented Jul 30, 2018

View reviewed changes

EdricChan03 reviewed Jul 31, 2018

View reviewed changes

vivian-hu-zz force-pushed the firstbug branch from 0f5d2ee to b66051d Compare July 31, 2018 17:18

jelbourn reviewed Jul 31, 2018

View reviewed changes

vivian-hu-zz force-pushed the firstbug branch from b66051d to 88e48b7 Compare August 2, 2018 17:42

vivian-hu-zz commented Aug 2, 2018

View reviewed changes

vivian-hu-zz force-pushed the firstbug branch from 88e48b7 to 23b26b0 Compare August 2, 2018 18:39

jelbourn reviewed Aug 2, 2018

View reviewed changes

vivian-hu-zz force-pushed the firstbug branch 2 times, most recently from 84b95c2 to 36e534c Compare August 3, 2018 22:59

vivian-hu-zz commented Aug 3, 2018

View reviewed changes

jelbourn approved these changes Aug 4, 2018

View reviewed changes

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker merge: fix commit message When the PR is merged, rewrites/fixups of the commit messages are needed labels Aug 4, 2018

vivian-hu-zz force-pushed the firstbug branch from 36e534c to 8d07367 Compare August 7, 2018 23:54

vivian-hu-zz changed the title ~~feat(list): add button as the underlining element for a list item~~ feat(list): add new '<mat-action-list>' Aug 7, 2018

josephperrott reviewed Aug 9, 2018

View reviewed changes

vivian-hu-zz force-pushed the firstbug branch from 8d07367 to 2cfed42 Compare August 10, 2018 16:44

vivian-hu-zz commented Aug 10, 2018

View reviewed changes

josephperrott approved these changes Aug 16, 2018

View reviewed changes

Vivian Hu added 5 commits August 31, 2018 15:13

feat(list): add button as the underlining element for a list item

36dd04b

simple list only in this check in

address code review comment

257ca2f

address review comment part2

3f84eb9

more change based on code review

28b7145

feat(list): add new '<mat-action-list>'

90fdc16

An action-list is a list where each list item is a native '<button>' element, meant to be used in cases where each list item has a single action with no sub-action, navigation or selection.

vivian-hu-zz force-pushed the firstbug branch from 2cfed42 to 90fdc16 Compare August 31, 2018 22:17

jelbourn merged commit 69fa762 into angular:master Sep 11, 2018

mitchellwills mentioned this pull request Oct 10, 2018

mat-action-list #9429

Closed

vivian-hu-zz deleted the firstbug branch January 23, 2019 23:00

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

feat(list): add new '<mat-action-list>' #12415

feat(list): add new '<mat-action-list>' #12415

Conversation

vivian-hu-zz commented Jul 27, 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

vivian-hu-zz left a comment

Choose a reason for hiding this comment

EdricChan03 Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

EdricChan03 Jul 31, 2018 • 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

EdricChan03 Jul 31, 2018 • 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

ngbot bot commented Jul 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vivian-hu-zz left a comment • edited by jelbourn Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vivian-hu-zz left a comment

Choose a reason for hiding this comment

jelbourn left a comment

Choose a reason for hiding this comment

jelbourn commented Aug 7, 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

vivian-hu-zz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngbot bot commented Aug 31, 2018

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

EdricChan03 Jul 31, 2018 •

edited

Loading

EdricChan03 Jul 31, 2018 •

edited

Loading

EdricChan03 Jul 31, 2018 •

edited

Loading

vivian-hu-zz left a comment •

edited by jelbourn

Loading