feat(list): Add single selection #2970

williamernest · 2018-06-20T17:55:48Z

No description provided.

codecov-io · 2018-06-20T18:21:17Z

Codecov Report

Merging #2970 into master will increase coverage by <.01%.
The diff coverage is 98.91%.

@@            Coverage Diff             @@
##           master    #2970      +/-   ##
==========================================
+ Coverage    98.3%   98.31%   +<.01%     
==========================================
  Files         101      101              
  Lines        4368     4451      +83     
  Branches      564      585      +21     
==========================================
+ Hits         4294     4376      +82     
- Misses         74       75       +1

Impacted Files	Coverage Δ
packages/mdc-list/constants.js	`100% <ø> (ø)`	⬆️
packages/mdc-list/foundation.js	`100% <100%> (ø)`	⬆️
packages/mdc-list/index.js	`98.78% <97.72%> (-1.22%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c1fa93a...f1bce52. Read the comment docs.

patrickrodee · 2018-06-20T20:19:24Z

package-lock.json

@@ -6176,6 +6176,7 @@
          "version": "0.0.9",
          "bundled": true,
          "dev": true,
+          "optional": true,


Was this file supposed to change?

patrickrodee · 2018-06-20T20:21:30Z

packages/mdc-list/README.md

@@ -212,7 +254,11 @@ Method Signature | Description
 `getListItemCount() => Number` | Returns the total number of list items (elements with `mdc-list-item` class) that are direct children of the `root_` element.
 `getFocusedElementIndex() => Number` | Returns the `index` value of the currently focused element.
 `getListItemIndex(ele: Element) => Number` | Returns the `index` value of the provided `ele` element.
+`setAttributeForElementIndex(ndx: Number, attr: String, value: String) => void` | Sets the `attr` attribute to the value of `value` for the list item at `ndx`.


I'm not in love with the variable name ndx but it's already in here so I'm not gonna request a change.

patrickrodee · 2018-06-20T20:30:05Z

packages/mdc-list/foundation.js

+    if (ndx === this.selectedIndex_) {
+      this.adapter_.setAttributeForElementIndex(this.selectedIndex_, strings.ARIA_SELECTED, false);
+      this.adapter_.removeClassForElementIndex(this.selectedIndex_, cssClasses.LIST_SELECTED_CLASS);
+      if (this.selectedIndex_ > 0) {


Why does this only set negative tabindex for items 1...n instead of 0...n?

patrickrodee · 2018-06-20T20:30:44Z

packages/mdc-list/foundation.js

+    }
+
+    if (ndx >= 0) {
+      if (this.adapter_.getListItemCount() > ndx) {


Collapse these into one if statement

patrickrodee · 2018-06-20T20:37:31Z

packages/mdc-list/foundation.js

+    } else if (this.isSingleSelectionList_ && (isEnter || isSpace)) {
+      this.preventDefaultEvent_(evt);
+      // Check if the space key was pressed on the list item or a child element.
+      if (evt.target.classList.contains(cssClasses.LIST_ITEM_CLASS)) {


This should be an adapter method (hasClass or something similar)

patrickrodee · 2018-06-20T20:38:23Z

packages/mdc-list/foundation.js

+
+      currentIndex = this.adapter_.getListItemIndex(listItem);
+
+      if (currentIndex < 0) {


This if statement can be removed since it always evaluates to true.

patrickrodee · 2018-06-20T20:40:07Z

packages/mdc-list/index.js

+    }
+
+    this.foundation_.setSingleSelection(value);
+    const selectedElement = this.root_.querySelector('.mdc-list-item[aria-selected="true"]');


Could you provide some explanation about why this uses the aria-selected attribute instead of the mdc-list-item--selected class?

patrickrodee · 2018-06-20T20:40:27Z

packages/mdc-list/index.js

+
+    this.foundation_.setSingleSelection(value);
+    const selectedElement = this.root_.querySelector('.mdc-list-item[aria-selected="true"]');
+    [].slice.call(this.root_.querySelectorAll('.mdc-list-item:not([aria-selected="true"])'))


Could you provide some explanation about why this uses the aria-selected attribute instead of the mdc-list-item--selected class?

patrickrodee · 2018-06-20T20:41:34Z

packages/mdc-list/index.js

+        if (!matches) { // IE uses a different name for the same functionality
+          matches = Element.prototype.msMatchesSelector;
+        }
+        return matches.call(ele, strings.FOCUSABLE_CHILD_ELEMENTS);


patrickrodee · 2018-06-21T17:10:36Z

packages/mdc-list/README.md

+#### Pre-selected list item
+
+When rendering the list with a pre-selected list item, the list item that needs to be selected should contain
+the `mdc-list-item--selected` class before creating the list.


Should the pre-selected list item also have the aria-selected="true" attribute?

No, it gets added automatically when the list component is created. We add the --selected class because we use that as our selector.

Gotcha. Can you show me the line number where that occurs?

material-components-web/packages/mdc-list/index.js

Line 106 in cbbd394

const selectedElement = this.root_.querySelector('.mdc-list-item--selected');

I don't see where aria-selected="true" gets added though. Could you point me to that?

Cool, that makes sense. I still don't see how the default selected list item gets the aria-selected attribute though. As far as I can tell, it never gets the aria-selected attribute.

If the selectedElement is found it sets the selectedIndex to the selectedElement index. The selectedIndex proxies to the foundation setSelectedIndex, which sets the attributes on the list element.

material-components-web/packages/mdc-list/index.js

Line 111 in cbbd394

this.selectedIndex = this.listElements_.indexOf(selectedElement);

material-components-web/packages/mdc-list/index.js

Line 117 in cbbd394

this.foundation_.setSelectedIndex(index);

material-components-web/packages/mdc-list/foundation.js

Line 115 in cbbd394

this.adapter_.setAttributeForElementIndex(this.selectedIndex_, strings.ARIA_SELECTED, true);

Awesome, thanks for the patience and guiding me through. Since that means the aria-selected attribute is only set by the MDC component index.js implementation during initialization, let's add a note to include aria-selected="true" for the pre-selected list item. That will help users who wrap our components and our users in general.

? aria-selected is set by the foundation

If you wrap the component and want to use the single selection list, you have to call setSelectedIndex on the foundation so that it can initialize the initial selected value. setSelectedIndex also sets aria-selected on the list element that is selected.

patrickrodee · 2018-06-28T16:56:10Z

packages/mdc-list/adapter.js

@@ -41,11 +41,48 @@ class MDCListAdapter {
  /** @param {Element} node */
  getListItemIndex(node) {}

+  /**
+   * @param {Number} index


These basic types (number, string, boolean, etc) should be lowercase.

patrickrodee · 2018-06-28T16:57:11Z

packages/mdc-list/constants.js

  FOCUSABLE_CHILD_ELEMENTS: 'button:not(:disabled), a',
-  ITEMS_SELECTOR: '.mdc-list-item',
+  ITEMS_SELECTOR: '.mdc-list-item:not(.mdc-list-item--disabled)',


Why did you add the :not(.mdc-list-item--disabled) selector?

Disabled items should not be receive focus, so the items selector should ignore them.

Changing the key to ENABLED_ITEMS_SELECTOR is more semantically correct

patrickrodee · 2018-06-28T17:03:47Z

packages/mdc-list/index.js

+  /** @param {boolean} isSingleSelectionList */
+  set singleSelection(isSingleSelectionList) {
+    if (isSingleSelectionList) {
+      this.root_.addEventListener('click', this.handleClick_);


This should be a foundation method.

Aren't we moving toward adding event listeners in the index.js ? See initialSyncWithDOM above.

True, we're exploring it. I guess we can try it here and see how it goes.

Will handleClick cause problems if it's triggered by a non-single selection list item? If so, there should be a check inside the handleClick method that ensures it won't cause problems if it's called.

handleClick has logic to determine if you did not click on a list item (it looks for an ancestor mdc-list-item) and if doesn't find one it returns early.

patrickrodee · 2018-06-28T19:02:21Z

packages/mdc-list/index.js

+  /** @param {boolean} isSingleSelectionList */
+  set singleSelection(isSingleSelectionList) {
+    if (isSingleSelectionList) {
+      this.root_.addEventListener('click', this.handleClick_);


True, we're exploring it. I guess we can try it here and see how it goes.

Will handleClick cause problems if it's triggered by a non-single selection list item? If so, there should be a check inside the handleClick method that ensures it won't cause problems if it's called.

trimox · 2018-07-04T14:16:55Z

Hey @williamernest I opened PR #3029 to export MDCListFoundation. Would you prefer to add those exports here? If so, I'll close #3029.

williamernest · 2018-07-06T20:27:32Z

@trimox Thanks. I updated them here so we can go ahead and close #3029

moog16

Left some comments on tests, which you can ignore except for one :)

moog16 · 2018-07-13T23:52:31Z

packages/mdc-list/README.md

+
+The default component requires that every list item receives a `tabindex` value so that it can receive focus
+(`li` elements cannot receive focus at all without a `tabindex` value). Any element not already containing a
+`tabindex` attribute will receive `tabindex=-1`. The first list item should have `tabindex="0"` so that it the


so that the user...

moog16 · 2018-07-13T23:55:42Z

packages/mdc-list/README.md

+(`li` elements cannot receive focus at all without a `tabindex` value). Any element not already containing a
+`tabindex` attribute will receive `tabindex=-1`. The first list item should have `tabindex="0"` so that it the
+user can find the first element using the `tab` key, but subsequent `tab` keys strokes will cause focus to
+skip over the entire list. If the list items contain sub-elements that are focusable (`button` or `a` elements), 


is it also true that inputs will receive a tabindex='-1'?

No, we only query for button and a elements to toggle tabIndex. We removed the checkbox demos so we could focus on the base cases (button and a elements) and then add the other features later.

oh sorry - i meant to remove this comment.

moog16 · 2018-07-14T00:02:17Z

packages/mdc-list/README.md

+  <li class="mdc-list-item mdc-list-item--selected" aria-selected="true" tabindex="0">Single-line item</li>
+  <li class="mdc-list-item" tabindex="-1">Single-line item</li>
+</ul>
+```


I think adding the JS here showing to call setSelectedIndex will complete the documentation....if I'm reading that correctly.

The documentation shows the JS in above examples. This example is discussing how the html should be rendered when creating your own component.

moog16 · 2018-07-14T00:06:37Z

packages/mdc-list/constants.js

@@ -18,14 +18,16 @@
 /** @enum {string} */
 const cssClasses = {
  LIST_ITEM_CLASS: 'mdc-list-item',
+  LIST_SELECTED_CLASS: 'mdc-list-item--selected',


LIST_ITEM_SELECTED_CLASS is more consistent with the above class

moog16 · 2018-07-14T00:07:54Z

packages/mdc-list/constants.js

  FOCUSABLE_CHILD_ELEMENTS: 'button:not(:disabled), a',
-  ITEMS_SELECTOR: '.mdc-list-item',
+  ITEMS_SELECTOR: '.mdc-list-item:not(.mdc-list-item--disabled)',


Changing the key to ENABLED_ITEMS_SELECTOR is more semantically correct

moog16 · 2018-07-14T00:36:38Z

test/unit/mdc-list/mdc-list.test.js

+  document.body.appendChild(root);
+  const selectedNode = root.querySelectorAll('.mdc-list-item')[1];
+  component.getDefaultFoundation().adapter_.removeClassForElementIndex(1, 'foo');
+  assert.isFalse(selectedNode.classList.contains('foo'));


when does the element get the foo class? I ran this and it looks like if you remove line 113 it still passes

moog16 · 2018-07-14T00:38:24Z

test/unit/mdc-list/foundation.test.js

+  foundation.handleKeydown(event);
+
+  td.verify(preventDefault(), {times: 1});
+  td.verify(mockAdapter.setAttributeForElementIndex(0, strings.ARIA_SELECTED, true));


do you want a {times: 1} here?

{times: 1} is the default behavior.

I'm pretty sure that is not correct. The default behavior is any non-zero number of invocations is valid.

test/unit/mdc-list/foundation.test.js

+  foundation.handleKeydown(event);
+
+  td.verify(preventDefault(), {times: 2});
+  td.verify(mockAdapter.removeAttributeForElementIndex(0, strings.ARIA_SELECTED));


williamernest · 2018-07-16T17:17:57Z

test/unit/mdc-list/foundation.test.js

+  const {foundation, mockAdapter} = setupTest();
+  const preventDefault = td.func('preventDefault');
+  const target = {classList: ['mdc-list-item']};
+  const event = {key: 'Space', target, preventDefault};


It doesn't doesn't increase coverage since that statement is already covered by the key parameter.

moog16 · 2018-07-14T00:44:06Z

test/unit/mdc-list/mdc-list.test.js

+  td.verify(mockFoundation.setSingleSelection(true), {times: 1});
+});
+
+test('selectedIndex calls setSelectedIndex on foundation', () => {


setSelectedIndex is a pretty complex function, but only has this one test. Might want to add more

setSelectedIndex has nearly 100% coverage through the click/key handler tests.

moog16

2 comments about test titles. I think they are off. BUT everything else looks good pending those changes.

moog16 · 2018-07-16T20:32:26Z

test/unit/mdc-list/foundation.test.js

+  td.verify(preventDefault(), {times: 1});
+});
+
+test('#handleKeydown enter key causes preventDefault to be called on the event', () => {


I think you have the titles of this and the next tests reversed with regards to if singleSelection=true

moog16 · 2018-07-16T20:32:43Z

test/unit/mdc-list/foundation.test.js

+  td.verify(preventDefault(), {times: 0});
+});
+
+test('#handleKeydown enter key does not causes preventDefault to be called if singleSelection=true', () => {


same here - titles reversed

Review by moog16

williamernest and others added 23 commits May 30, 2018 07:37

WIP: Temp

b0a619b

feat(list): Add arrow key accessibility

435c2d4

feat(list): Update demo and key handler logic

87ab27e

feat(list): Update

0ec36f3

feat(list): Update

0d80d3e

feat(list): Update readme

bf18a37

feat(list): Update test

efb7449

Merge branch 'master' into feat/list/new-component

4746b68

feat(list): Update

adc1ece

Merge branch 'master' into feat/list/new-component

1abc9fa

feat(list): Update for comments

1284480

feat(list): Update for comments

9580532

WIP: Temp

da21fa2

WIP: Temp

c11fa07

feat(list): Add single-selection to list

17ca8be

feat(list): Update tests

d175876

feat(list): Add unit tests

6539e70

feat(list): Update tests

fe5d7db

feat(list): Readme and demo update

b2dc484

feat(list): Update closure and fix bug with adapter method

23427af

feat(list): Update for closure

c9c9ce9

feat(list): Merge master

049af1d

Merge branch 'master' into feat/list/aria-selected

1d5f610

patrickrodee requested changes Jun 20, 2018

View reviewed changes

patrickrodee self-assigned this Jun 20, 2018

williamernest and others added 3 commits June 20, 2018 15:38

feat(list): Update for comments

432863f

feat(list): Update for comments

d13d2f7

Merge branch 'master' into feat/list/aria-selected

c6fd003

patrickrodee requested changes Jun 21, 2018

View reviewed changes

feat(list): Fix lint

2c8ccf0

patrickrodee requested changes Jun 28, 2018

View reviewed changes

williamernest and others added 3 commits June 28, 2018 10:17

feat(list): Update for comments

e86e629

feat(list): Update for comments

b3ba9aa

Merge branch 'master' into feat/list/aria-selected

fd3dc39

patrickrodee previously requested changes Jun 28, 2018

View reviewed changes

feat(list): Reverse not selector for list states

98657d6

feat(list): Update exports, defaults and imports

400c2ba

williamernest mentioned this pull request Jul 6, 2018

fix(list): Add MDCListFoundation export #3029

Closed

feat(list): Remove selection demo

bc597e7

trimox mentioned this pull request Jul 7, 2018

Add single selection to List trimox/angular-mdc-web#1084

Closed

Update README.md

fb6b1a4

moog16 suggested changes Jul 14, 2018

View reviewed changes

williamernest and others added 4 commits July 16, 2018 10:18

feat(list): Update for comments. Add tests. Add demo

f7482f0

Merge branch 'master' into feat/list/aria-selected

ff0bee6

feat(list): Update for lint

81569dc

Merge branch 'master' into feat/list/aria-selected

9bb119b

moog16 approved these changes Jul 16, 2018

View reviewed changes

williamernest and others added 3 commits July 16, 2018 14:14

feat(list): Update for comments

92aa3a6

Merge branch 'master' into feat/list/aria-selected

2bf7472

Merge branch 'master' into feat/list/aria-selected

f1bce52

williamernest merged commit cd1f972 into master Jul 16, 2018

williamernest deleted the feat/list/aria-selected branch July 16, 2018 21:45

jamesmfriedman mentioned this pull request Jul 30, 2018

MDC 38/39 rmwc/rmwc#281

Closed

48 tasks

williamernest mentioned this pull request Jul 30, 2018

List - handleClick method is missing from foundation #3257

Closed

trimox mentioned this pull request Jul 30, 2018

Update to material-components-web v0.38.0 trimox/angular-mdc-web#1162

Closed

williamernest mentioned this pull request Jul 31, 2018

mdc-list should support a single aria-selected item #2851

Closed

3 tasks


		currentIndex = this.adapter_.getListItemIndex(listItem);

		if (currentIndex < 0) {

feat(list): Add single selection #2970

feat(list): Add single selection #2970

Conversation

williamernest commented Jun 20, 2018

codecov-io commented Jun 20, 2018 • edited Loading

Codecov Report

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

patrickrodee Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williamernest Jun 21, 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

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

williamernest Jun 28, 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

trimox commented Jul 4, 2018

williamernest commented Jul 6, 2018

moog16 left a 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

This comment was marked as resolved.

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moog16 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jun 20, 2018 •

edited

Loading

patrickrodee Jun 21, 2018 •

edited

Loading

williamernest Jun 21, 2018 •

edited

Loading

williamernest Jun 28, 2018 •

edited

Loading