feat(list): Toggle radio checkbox #3546

williamernest · 2018-09-11T21:50:08Z

BREAKING CHANGE: Moves the toggling of radio buttons and checkboxes to the list from the menu. Adds toggleCheckbox List adapter API.

codecov-io · 2018-09-11T22:03:21Z

Codecov Report

Merging #3546 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3546      +/-   ##
==========================================
+ Coverage   98.35%   98.35%   +<.01%     
==========================================
  Files         120      120              
  Lines        5168     5175       +7     
  Branches      640      642       +2     
==========================================
+ Hits         5083     5090       +7     
  Misses         85       85

Impacted Files	Coverage Δ
packages/mdc-menu/foundation.js	`100% <ø> (ø)`	⬆️
packages/mdc-list/constants.js	`100% <ø> (ø)`	⬆️
packages/mdc-menu/index.js	`100% <ø> (ø)`	⬆️
packages/mdc-list/foundation.js	`99.16% <100%> (+0.04%)`	⬆️
packages/mdc-list/index.js	`100% <100%> (ø)`	⬆️

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 26f6699...59008b7. Read the comment docs.

mdc-web-bot · 2018-09-11T22:18:47Z

All 389 screenshot tests passed for commit 73afd56 vs. master! 💯🎉

mdc-web-bot · 2018-09-11T23:58:17Z

All 389 screenshot tests passed for commit 2ec343b vs. master! 💯🎉

kfranqueiro

Discussed offline: Radio buttons have different behavior than checkboxes with regard to emitting change events (the change event only emits on the radio that becomes checked), and it might not make sense to specifically apply toggle logic to radios the same way we're handling checkboxes.

…issue

kfranqueiro · 2018-09-13T17:25:26Z

packages/mdc-list/index.js

+          const event = document.createEvent('Event');
+          event.initEvent('change', false, true);
+
+          if (!element.checked) {


I think this can be simplified to if (!element.checked || element.type !== 'radio') and avoid needing the else and repeating the 2 lines inside? (Move the comment up)

kfranqueiro · 2018-09-13T17:26:17Z

packages/mdc-list/index.js

+        const listItem = this.listElements[index];
+        const elementsToToggle =
+          [].slice.call(listItem.querySelectorAll('input[type="radio"], input[type="checkbox"]'));
+        elementsToToggle.forEach((element) => {


If we let a boolean outside of this loop and set it to true when this function is run, we can return it as an indicator of whether an input was found to toggle, and preventDefault on the event within the foundation if this returns true.

kfranqueiro · 2018-09-17T16:14:27Z

packages/mdc-menu/adapter.js

   */
-  toggleCheckbox(target) {}
+  toggleCheckbox(index) {}


This is still in menu's adapter... it belongs in List's now, right?

kfranqueiro · 2018-09-17T16:16:53Z

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

+test('adapter#toggleCheckbox toggles a radio button', () => {
+  const {root, component} = setupTest();
+  document.body.appendChild(root);
+  const checkbox = root.querySelector('input[type="radio"]');


Nit: checkbox -> radio

kfranqueiro · 2018-09-17T16:20:07Z

packages/mdc-list/index.js

+          [].slice.call(listItem.querySelectorAll('input[type="radio"], input[type="checkbox"]'));
+        elementsToToggle.forEach((element) => {
+          const event = document.createEvent('Event');
+          event.initEvent('change', false, true);


I think there are some potential issues here...

Why is bubbles set to false? Change events bubble by default. i.e. if I set up a document-level change listener, I will only get change events if I actually interact directly with the input right now.

However, right now, if I click directly on a checkbox, I get 2 change events emitted directly on the checkbox. This doesn't happen for radios, possibly because of the if below.

Also, initEvent is apparently deprecated in favor of directly using Event constructors; see https://developer.mozilla.org/en-US/docs/Web/Guide/Events/Creating_and_triggering_events

Event constructors are not supported in IE11.
https://developer.mozilla.org/en-US/docs/Web/API/Event/Event

kfranqueiro · 2018-09-17T16:34:54Z

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

-  td.verify(mockFoundation.handleClick(td.matchers.anything()), {times: 1});
-});
-
-test('singleSelection false removes the click handler from the root element', () => {


FYI there is no equivalent test to this in the new code (i.e. no test that tests MDCListFoundation#handleClick when isSingleSelectionList_ is false)

kfranqueiro · 2018-09-17T16:35:55Z

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

@@ -374,3 +358,23 @@ test('keydown handler is removed from the root element on destroy', () => {
  listElementItem.dispatchEvent(event);
  td.verify(mockFoundation.handleKeydown(event, true, 0), {times: 0});
 });
+
+test('adapter#toggleCheckbox toggles a checkbox', () => {


Add a third test with no checkbox/radio to verify it returns false

kfranqueiro · 2018-09-18T18:35:38Z

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

+  td.verify(mockAdapter.setAttributeForElementIndex(0, 'tabindex', 0), {times: 0});
+});
+
+test('#handleClick proxies to the adapter#toggleCheckbox is toggleCheckbox is true', () => {


is toggleCheckbox -> if toggleCheckbox

kfranqueiro · 2018-09-18T18:36:15Z

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

@@ -481,62 +481,66 @@ test('#handleKeydown space key is triggered and focused is moved to a different
  td.verify(mockAdapter.setAttributeForElementIndex(2, 'tabindex', 0), {times: 1});
 });

+test('#handleClick when singleSelection=true on a list item should not cause the list item to be selected', () => {


true -> false

kfranqueiro · 2018-09-18T18:45:29Z

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

-    component.getDefaultFoundation().adapter_.setTabIndexForListItemChildren(index, 0);
-    assert.equal(1, listItems[index].querySelectorAll('[tabindex="0"]').length);
-  }
+  component.getDefaultFoundation().adapter_.setTabIndexForListItemChildren(0, 0);


The descriptions of this test and in the README need updating for the new behavior.

kfranqueiro · 2018-09-18T18:54:23Z

FYI there's currently no test exercising the preventDefault branch in handleKeydown.

kfranqueiro · 2018-09-19T17:31:33Z

packages/mdc-list/README.md

-and then tab to the first element after the list. The `Arrow`, `Home`, and `End` keys should be used for navigating
-internal list elements. If `singleSelection=true`, the list will allow the user to use the `Space` or `Enter` keys to
-select or deselect a list item. The MDCList will perform the following actions for each key press
+As the user navigates through the list, any `button`, and `a` elements within the list will receive `tabindex="-1"` when


Nit: remove comma between "button and a"

kfranqueiro · 2018-09-19T17:33:56Z

packages/mdc-list/foundation.js

@@ -203,17 +204,31 @@ class MDCListFoundation extends MDCFoundation {
        this.adapter_.followHref(currentIndex);
      }
    }
+
+    if (isEnter || isSpace) {


Would it be more straightforward to put this within the else if above, make that else if check only for isEnter || isSpace, then nest if (this.isSingleSelectionList_) within that? IMO it's a little hard to follow right now since there's two separate blocks with logic for enter/space, and potentially both of them can be executed.

Tangentially, I also can't remember why the preventDefault for single-selection is outside the isRootListItem check?

kfranqueiro · 2018-09-19T17:40:53Z

packages/mdc-menu/adapter.js

@@ -110,19 +110,6 @@ class MDCMenuAdapter {
 *   }} evtData
   */
  notifySelected(evtData) {}
-


These also need to be removed from the menu README

kfranqueiro

LGTM with one nit and one test comment

kfranqueiro · 2018-09-19T21:30:40Z

packages/mdc-list/foundation.js

-      if (isRootListItem) {
-        this.setSelectedIndex(currentIndex);
+    } else if (isEnter || isSpace) {
+      if (this.isSingleSelectionList_) {


I think we can collapse these two nested if's (single-selection + root list item) into one if && now?

kfranqueiro · 2018-09-19T21:30:57Z

packages/mdc-list/foundation.js

+    } else if (isEnter || isSpace) {
+      if (this.isSingleSelectionList_) {
+        // Check if the space key was pressed on the list item or a child element.
+        if (isRootListItem) {


We don't seem to have a test for single-selection + non-root-list-item

williamernest added 4 commits September 11, 2018 13:37

WIP: Temp

e8cb2b0

WIP: Temp

86e312f

WIP: Update tests

51d190a

WIP: Remove dead code from demo

79bf0e1

googlebot added the cla: yes label Sep 11, 2018

Merge branch 'master' into feat/list/toggle-radio-checbox

73afd56

WIP: Fix lint

2ec343b

kfranqueiro self-requested a review September 12, 2018 01:20

kfranqueiro self-assigned this Sep 12, 2018

kfranqueiro reviewed Sep 12, 2018

View reviewed changes

fix(list): Update for radio groups. Add demo. Fix label double check …

18338a3

…issue

kfranqueiro suggested changes Sep 13, 2018

View reviewed changes

williamernest and others added 3 commits September 13, 2018 10:39

fix(list): Update for comments

f1f54c9

WIP: Update demo

9882e24

Merge branch 'master' into feat/list/toggle-radio-checbox

413b607

abhiomkar mentioned this pull request Sep 14, 2018

feat(list): Added setFocusInListItemChildren new API to list foundation. #3564

Closed

kfranqueiro suggested changes Sep 17, 2018

View reviewed changes

kfranqueiro reviewed Sep 17, 2018

View reviewed changes

williamernest and others added 3 commits September 17, 2018 10:50

WIP: Update for comments

3c207d1

WIP: Allow change event to bubble

93de525

Merge branch 'master' into feat/list/toggle-radio-checbox

9dcd1fa

abhiomkar mentioned this pull request Sep 17, 2018

MDCList: Make toggling tabindex of interactive elements optional #3511

Closed

williamernest and others added 4 commits September 17, 2018 16:59

WIP: Update for comments

ad2f7d5

WIP: Clean up merge issue

2da3803

Merge branch 'master' into feat/list/toggle-radio-checbox

76652e0

WIP: Update tests

5371201

williamernest and others added 3 commits September 18, 2018 09:48

WIP: Update test branch coverage

e28d34e

Merge branch 'master' into feat/list/toggle-radio-checbox

aa68248

WIP: Fix closure

b4af056

kfranqueiro suggested changes Sep 18, 2018

View reviewed changes

williamernest and others added 2 commits September 19, 2018 10:16

WIP: Update for commentS

3a978fb

Merge branch 'master' into feat/list/toggle-radio-checbox

3e98303

kfranqueiro suggested changes Sep 19, 2018

View reviewed changes

WIP: Update for comments

3ca6457

kfranqueiro approved these changes Sep 19, 2018

View reviewed changes

williamernest and others added 2 commits September 19, 2018 16:54

WIP: Update for comments. Add tests

8c09bc2

Merge branch 'master' into feat/list/toggle-radio-checbox

59008b7

williamernest merged commit f59b6e6 into master Sep 20, 2018

williamernest deleted the feat/list/toggle-radio-checbox branch September 20, 2018 00:53

kfranqueiro mentioned this pull request Sep 20, 2018

Integrate List component with Dialog in docs and screenshot tests #3592

Closed

jamesmfriedman mentioned this pull request Sep 26, 2018

MDC 0.40.0 rmwc/rmwc#323

Closed

49 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(list): Toggle radio checkbox #3546

feat(list): Toggle radio checkbox #3546

williamernest commented Sep 11, 2018 •

edited by kfranqueiro

Loading

codecov-io commented Sep 11, 2018 •

edited

Loading

mdc-web-bot commented Sep 11, 2018

mdc-web-bot commented Sep 11, 2018

kfranqueiro left a comment

kfranqueiro Sep 13, 2018

kfranqueiro Sep 13, 2018

kfranqueiro Sep 17, 2018

kfranqueiro Sep 17, 2018

kfranqueiro Sep 17, 2018

kfranqueiro Sep 17, 2018

williamernest Sep 17, 2018

kfranqueiro Sep 17, 2018

kfranqueiro Sep 17, 2018

kfranqueiro Sep 18, 2018

kfranqueiro Sep 18, 2018

kfranqueiro Sep 18, 2018

kfranqueiro commented Sep 18, 2018

kfranqueiro Sep 19, 2018

kfranqueiro Sep 19, 2018

kfranqueiro Sep 19, 2018

kfranqueiro left a comment

kfranqueiro Sep 19, 2018 •

edited

Loading

kfranqueiro Sep 19, 2018

feat(list): Toggle radio checkbox #3546

feat(list): Toggle radio checkbox #3546

Conversation

williamernest commented Sep 11, 2018 • edited by kfranqueiro Loading

codecov-io commented Sep 11, 2018 • edited Loading

Codecov Report

mdc-web-bot commented Sep 11, 2018

mdc-web-bot commented Sep 11, 2018

kfranqueiro 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

kfranqueiro commented Sep 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfranqueiro left a comment

Choose a reason for hiding this comment

kfranqueiro Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williamernest commented Sep 11, 2018 •

edited by kfranqueiro

Loading

codecov-io commented Sep 11, 2018 •

edited

Loading

kfranqueiro Sep 19, 2018 •

edited

Loading