refactor(list): Refactor list item management #413

bonniezhou · 2018-11-05T19:40:50Z

Move the logic having to do with list item classes, attributes, etc inside ListItem instead of in List. Move event handlers to ListItem. Remove the needs for IDs on list items, instead recalibrate the list item index on every re-render.

Fixes #367, #366, #365.

codecov-io · 2018-11-05T21:24:11Z

Codecov Report

Merging #413 into master will decrease coverage by 0.87%.
The diff coverage is 80.48%.

@@            Coverage Diff             @@
##           master     #413      +/-   ##
==========================================
- Coverage   96.94%   96.07%   -0.88%     
==========================================
  Files          53       53              
  Lines        1836     1811      -25     
  Branches      212      212              
==========================================
- Hits         1780     1740      -40     
- Misses         56       71      +15

Impacted Files	Coverage Δ
packages/list/ListGroup.js	`100% <ø> (ø)`	⬆️
packages/list/ListItem.js	`87.5% <71.42%> (-8.5%)`	⬇️
packages/list/index.js	`89.09% <82.35%> (-10.91%)`	⬇️

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 003221a...fbc15f1. Read the comment docs.

moog16 · 2018-11-05T21:33:04Z

packages/list/index.js

+      getFocusedElementIndex: () => {
+        let count = -1;
+        let index = -1;
+        React.Children.forEach(this.props.children, (child) => {


this is cool - didn't know you could inspect child.type.name. Aren't you give the index here? Is it because you're accounting for dividers and other elements that aren't ListItems?

Also is there a way to break early from this? If you've found the element

yep, the index thing is to get around dividers and non-ListItems. and you can't break early in React.Children.forEach so I changed it to use React.Children.toArray instead. https://stackoverflow.com/questions/51334473/react-how-to-break-out-of-react-children-foreach-loop

moog16 · 2018-11-05T21:33:19Z

packages/list/index.js

-      getFocusedElementIndex: () => this.getListItemIndexOfTarget_(document.activeElement),
+      getListItemCount: () => this.listItemCount,
+      getFocusedElementIndex: () => {
+        let count = -1;


I think count should start at 0

moog16 · 2018-11-05T21:38:23Z

packages/list/index.js

        }
+        const i = listItemClassNames[index].indexOf(className);
+        listItemClassNames[index].splice(i, 1);


I think you need to check if i<= -1. If you do listItemClassNames[index].splice(-1, 1), its going to delete something, which I believe is unintended.

moog16 · 2018-11-05T21:40:36Z

packages/list/index.js

-      }
-      target = target.parentElement;
+  renderChild = (child) => {
+    if (child.type.name === 'ListItem') {


I think in the Readme, we should say that the children need to be type ListItem's (if we don't already). One of the issues this past week gave an example of this:

<Link> <ListItem ... /> </Link>

moog16 · 2018-11-05T21:45:58Z

packages/list/index.js

-    },
-    this.state.listItemAttributes[idOrIndex]);
+      onKeyDown: (e) => {
+        onKeyDown();


pass the events in onKeyDown, onClick, onFocus, and onBlur. The user's may want that

moog16 · 2018-11-05T21:47:32Z

packages/list/index.js

+      shouldFocus: focusListItemAtIndex === index,
+      shouldFollowHref: followHrefAtIndex === index,
+      shouldToggleCheckbox: toggleCheckboxAtIndex === index,
+      attributesToSet: listItemAttributes[index],


How about renaming attributesToSet and classNamesToAdd to attributesFromList and classNamesFromList? I think it is inferred that we want to add them.

omg yes, I struggled hard with this naming 😅

moog16 · 2018-11-06T00:02:32Z

packages/list/index.js

    if (index >= 0) {
      this.foundation_.handleKeydown(e, true /* isRootListItem is true if index >= 0 */, index);
+      // Work around until MDC Web issue is resolved:


the if(index >= 0) can be inlined with if (e.key === 'Enter' || e.keyCode === 13 || e.key === 'Space' || e.keyCode === 32). The foundation.handleKeyDown method already does that check.

moog16 · 2018-11-06T00:02:58Z

packages/list/index.js

    // Toggle the checkbox only if it's not the target of the event, or the checkbox will have 2 change events.
    const toggleCheckbox = e.target.type === 'checkbox';
    this.foundation_.handleClick(index, toggleCheckbox);
+    // Work around until MDC Web issue is resolved:
+    // https://github.com/material-components/material-components-web/issues/4053
+    this.props.handleSelect(index);


this should have an if (index >= 0) check. foundation.handleClick does that check, so we should also do it here.

moog16 · 2018-11-06T00:03:51Z

packages/list/index.js

-    },
-    this.state.listItemAttributes[idOrIndex]);
+      onKeyDown: (e) => {
+        onKeyDown(e);


Sorry - should have thought of this earlier...do you think the index is useful information to pass to the onKeyDown event handler?

If we include index in onKeyDown then we should probably include it in the rest of the handlers too. I think we can leave it as is for now, and add it if someone has a specific use case for index.

bonniezhou · 2018-11-06T21:20:06Z

Filed #415

…md note * master: chore: Publish fix(text-field): added reference to input element (material-components#414) fix(drawer): update readme and screenshots to support below top app bar (material-components#397) refactor(list): Refactor list item management (material-components#413)

bonniezhou and others added 12 commits October 29, 2018 17:39

WIP refactor

2398167

more WIP

10e0618

WIP listItemPropUpdates state var

685eda0

WIP split up state vars again

833e7f7

omg it works

e7c6ce0

make it work with dividers

2f86c3b

add inserting list items to screenshot test

0f01fa7

TESTS

3ada718

lint

cc3a0bc

Merge branch 'master' into refactor/list-item-logic

96df39d

npm run capture

742ffa5

revert non list golden changes

6e66262

bonniezhou requested a review from moog16 November 5, 2018 20:58

fix test

768cb4f

moog16 suggested changes Nov 5, 2018

View reviewed changes

bonniezhou and others added 5 commits November 5, 2018 14:23

add handleSelect

a8bce9d

Merge branch 'master' into refactor/list-item-logic

2ab5e30

update for comments

f1bb2a1

merge with master

d373744

lint

9de4c28

moog16 suggested changes Nov 6, 2018

View reviewed changes

bonniezhou added 4 commits November 6, 2018 00:09

update golden

87231ce

revert golden for not list

a972255

remove .only from tet

b3afed1

update for comments

9d5a3ea

moog16 added this to the 0.6.x milestone Nov 6, 2018

moog16 self-assigned this Nov 6, 2018

bonniezhou added 2 commits November 5, 2018 17:02

fix some tests

88c0907

fix list group prop

9374018

bonniezhou and others added 2 commits November 6, 2018 08:56

Merge branch 'master' into refactor/list-item-logic

15e59f2

getFocusedElementIndex

48e6704

moog16 approved these changes Nov 6, 2018

View reviewed changes

merge with masteR

09b14c2

readme

fbc15f1

bonniezhou merged commit 1f563d3 into master Nov 6, 2018

bonniezhou deleted the refactor/list-item-logic branch November 6, 2018 22:11

This was referenced Nov 6, 2018

Move list event handlers from the list on to the list item #366

Closed

Add support for adding/removing list items #365

Closed

Error after removing ListItem #383

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(list): Refactor list item management #413

refactor(list): Refactor list item management #413

bonniezhou commented Nov 5, 2018

codecov-io commented Nov 5, 2018 •

edited

Loading

moog16 Nov 5, 2018

moog16 Nov 5, 2018

bonniezhou Nov 5, 2018

moog16 Nov 5, 2018

moog16 Nov 5, 2018

moog16 Nov 5, 2018

moog16 Nov 5, 2018

moog16 Nov 5, 2018

bonniezhou Nov 5, 2018

moog16 Nov 6, 2018

moog16 Nov 6, 2018

moog16 Nov 6, 2018

bonniezhou Nov 6, 2018

bonniezhou commented Nov 6, 2018

refactor(list): Refactor list item management #413

refactor(list): Refactor list item management #413

Conversation

bonniezhou commented Nov 5, 2018

codecov-io commented Nov 5, 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

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

bonniezhou commented Nov 6, 2018

codecov-io commented Nov 5, 2018 •

edited

Loading