feat(list): Add component #339

bonniezhou · 2018-10-16T01:11:27Z

Fixes #189

moog16

Still reviewing list/index.test.js, but wanted to send you this ahead of time.

moog16 · 2018-10-16T18:01:23Z

packages/list/ListItem.js

+ListItem.propTypes = {
+  childrenTabIndex: PropTypes.number,
+  className: PropTypes.string,
+  primaryText: PropTypes.string,


I think primaryText and secondaryText should be element, because they may want to wrap them in or something else.

actually it might be oneOfType(element, string)

can you give me an example? does MDC Web allow for text to be other elements?

There might be a text formatter or date formatter out there that you would use as a component. IE:

<ListItem primaryText={<DateFormatter date={new Date()} />} />

moog16 · 2018-10-16T18:06:58Z

packages/list/index.js

+      ref: (listItem) => !this.listItems_[index] && this.listItems_.push(listItem),
+      ...otherProps,
+    },
+    {className: this.getListItemClasses_(index)},


you can wrap these in one object:

{ updateClassList: this.updateListItemClassList, ref: (listItem) => !this.listItems_[index] && this.listItems_.push(listItem), className: this.getListItemClasses_(index) childrenTabIndex: this.state.listItemChildrenTabIndex[index], ...otherProps, },

I don't really know why but childrenTabIndex doesn't seem to be propagating if we do it this way

is it included in otherProps? If so, otherProps.childrenTabIndex will take precedence

got it, works now!

moog16 · 2018-10-16T18:09:00Z

packages/list/ListItem.js

+      ...otherProps
+    } = graphic.props;
+    const props = {
+      tabIndex: tabIndex != undefined ? this.props.childrenTabIndex : -1,


This is strange...shouldn't they be able to override tabIndex if they pass it through the graphic element? I don't see this logic in MDC Web list. Is this something that needs to be here?

Also !==

And if it is undefined doesn't the defaultProp take its place?

Yeah this is kinda weird...basically there are elements whose tabIndex we want the parent list item to be able to toggle between 0 and -1 (like button/icon/input/link), and elements whose tabIndex should always be -1 (like span/div/leading icons).

In MDC Web we just set button and a elements to be togglable, and all other elements to have tabIndex always -1. I think MDC Web is missing some functionality, for example icons should be togglable too.

So I wanted to make it up to the user to set the child elements to be togglable, while the rest default to be untogglable (tabIndex always -1). I think the idea is to completely control tabIndex within the list, meaning the user cannot override it. This is for accessibility. I redid it after splitting out the graphic/text/meta out into subcomponents, let me know what you think.

moog16 · 2018-10-16T18:15:05Z

packages/list/ListItem.js

+  renderText(primaryText, secondaryText) {
+    if (secondaryText) {
+      return (
+        <span className='mdc-list-item__text'>


I think we should make the follow separate elements that we provide:

mdc-list-item__text <ListItemText /> mdc-list-item__primary-text <ListItemPrimaryText /> mdc-list-item__secondary-text <ListItemSecondaryTexst /> mdc-list-item__graphic <ListItemGraphic /> mdc-list-item__meta <ListItemMeta />

OR we allow the user to do this both ways. So we can keep props inline, and provide the above elements, but then we would need to render children too.

I broke them out into 3 subcomponents because have 3 separate text components seemed overkill

moog16 · 2018-10-16T18:18:03Z

packages/list/index.js

+  }
+
+  componentDidMount() {
+    for (let i = 0; i < this.listItems_.length; i++) {


make this a forEach. It reads better.

moog16 · 2018-10-16T18:33:29Z

packages/list/index.js

+  }
+
+  getListItemIndexOfElement_ = (element) => {
+    for (let i = 0; i < this.listItems_.length; i++) {


I think you're using a for here, but what if we add the polyfill for findIndex? I remember you doing this somewwhere else too.

What polyfill?

moog16 · 2018-10-16T20:30:02Z

packages/list/index.js

+      removeAttributeForElementIndex: (index, attr) => {
+        const listItemAttributes = this.state.listItemAttributes;
+        attr = attr === 'tabindex' ? 'tabIndex' : attr;
+        listItemAttributes[index][attr] = null;


I think you actually have to remove it from the object?

moog16 · 2018-10-16T20:33:23Z

packages/list/index.js

+      toggleCheckbox: (index) => {
+        const listItem = this.listItems_[index];
+        if (listItem) {
+          listItem.toggleCheckbox();


still need to implement toggleCheckbox()

Yeah I think I'll file an issue and work on it separately

moog16 · 2018-10-16T20:36:37Z

packages/list/index.js

+    if (index >= 0) {
+      this.foundation_.handleKeydown(e, true /* isRootListItem is true if index >= 0 */, index);
+    }
+    this.props.onKeyDown(e);


Move the props.onEvent methods above. I think we should allow the developer to setup the dom before we do anything if needed. I cant think of why they would need to, but that's the pattern I've been following. What are your thoughts?

same with click, focus, and blur

moog16 · 2018-10-16T20:52:55Z

packages/list/index.js

+
+  getListItemClasses_(index) {
+    const {listItemClassList} = this.state;
+    return listItemClassList[index] ? classnames(Array.from(listItemClassList[index])) : '';


I think you also want to get the listItem.className and merge it here.

codecov-io · 2018-10-18T17:46:46Z

Codecov Report

Merging #339 into master will increase coverage by 0.19%.
The diff coverage is 99%.

@@            Coverage Diff             @@
##           master     #339      +/-   ##
==========================================
+ Coverage   97.29%   97.49%   +0.19%     
==========================================
  Files          40       45       +5     
  Lines        1514     1714     +200     
  Branches      164      191      +27     
==========================================
+ Hits         1473     1671     +198     
- Misses         41       43       +2

Impacted Files	Coverage Δ
packages/list/ListItemText.js	`100% <100%> (ø)`
packages/list/ListItemMeta.js	`100% <100%> (ø)`
packages/list/ListItemGraphic.js	`100% <100%> (ø)`
packages/list/ListItem.js	`96% <96%> (ø)`
packages/list/index.js	`99.29% <99.29%> (ø)`

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 f33a046...585f340. Read the comment docs.

moog16

testing some things in the screenshot tests...I will share if I find anything interesting.

moog16 · 2018-10-18T18:03:23Z

packages/list/ListItemGraphic.js

+    const graphicProps = {
+      className: this.classes,
+      role: 'presentation',
+      tabIndex: tabbableOnListItemFocus ? this.props.tabIndex : -1,


Why not just use tabIndex here if they pass a tabIndex in from props? Why do we need this special prop? This goes for all the other components that have this prop.

Talked offline: this is to determine whether the graphic tabIndex should switch from -1 to 0 when the list item is focused or if its tabIndex should always be -1.

moog16 · 2018-10-18T18:04:28Z

packages/list/ListItemGraphic.js

+
+    const graphicProps = {
+      className: this.classes,
+      role: 'presentation',


If the user passes in a different role, does this go with their configuration? We shouldn't overwrite.

It looks like this works:

<ListItemGraphic graphic={<MaterialIcon icon='folder'/>} role='menu'/>

But this looks more intuitive:

<ListItemGraphic graphic={<MaterialIcon icon='folder' role='menu'/>}/>

Good point, removed override.

moog16 · 2018-10-18T18:05:10Z

packages/list/ListItemMeta.js

+export default class ListItemMeta extends Component {
+  get classes() {
+    const {className, meta} = this.props;
+    return classnames('mdc-list-item__meta', className, meta.className);


I think if meta.className is undefined, it will print undefined in the rendered class

Just kidding...tested this. Its fine

moog16 · 2018-10-18T18:05:25Z

packages/list/ListItemMeta.js

+  tabbableOnListItemFocus: PropTypes.bool,
+  className: PropTypes.string,
+  tabIndex: PropTypes.number,
+  meta: PropTypes.element,


this should also be oneOfType(element, string)

moog16 · 2018-10-18T18:08:19Z

packages/list/ListItemText.js

+import PropTypes from 'prop-types';
+import classnames from 'classnames';
+
+export default class ListItemText extends Component {


I'm thinking we could even break this into . Not sure what you think. We'd also then need to provide another component that wraps the 2 (the element with .mdc-list-item__text). This might be simpler, so I'm all for it and just wanted to get your thoughts.

Talked offline, will keep as is unless there proves to be a need otherwise

moog16 · 2018-10-18T18:15:52Z

packages/list/index.js

+  };
+
+  componentDidMount() {
+    for (var key in this.listItems_) {


use let -- its just so in right now

moog16 · 2018-10-18T18:19:10Z

packages/list/index.js

+    const {listItemAttributes} = this.state;
+    listItemAttributes[key] = {
+      'aria-selected': false,
+      'tabIndex': this.listItemIndices_[key] === 0 ? 0 : -1,


what if someone manually changes the tabIndex to something other than 0 or -1...in this case it'll default to -1

moog16 · 2018-10-18T18:25:54Z

packages/list/index.js

+      ...otherProps
+    } = listItem.props;
+
+    const keyOrIndex = key || index;


Why do you have 1 or the other? I think the key should always just be the index. If you do this, you should be able to remove listItemIndicies.

moog16 · 2018-10-18T18:28:24Z

packages/list/index.js

+      setAttributeForElementIndex: (index, attr, value) => {
+        const {listItemAttributes} = this.state;
+        attr = attr === 'tabindex' ? 'tabIndex' : attr;
+        const key = this.getListItemKeyFromIndex_(index);


I think the key should always be the index. Instead of this, cant you just do this.listItems_[key]?

moog16 · 2018-10-18T18:29:39Z

packages/list/index.js

+
+  getListItemIndexOfTarget_ = (eventTarget) => {
+    // Find the first ancestor that is a list item.
+    const listItemElement = eventTarget.closest('.mdc-list-item');


not supported on IE :(

https://developer.mozilla.org/en-US/docs/Web/API/Element/closest

If it helps, the mdc-dom package's ponyfill module has this function

moog16 · 2018-10-23T20:17:52Z

packages/list/ListItem.js

@@ -27,6 +27,11 @@ import classnames from 'classnames';
 export default class ListItem extends Component {
  listItemElement_ = React.createRef();

+  componentDidMount() {
+    const {init} = this.props;


just add a defaultProp for init as an empty method. You don't need to guard against it on line 32.

init();

If we set a default prop here the actual prop doesn't override it....

moog16 · 2018-10-23T20:24:41Z

packages/list/README.md

 className | String | Classes to be applied to the list item element
 childrenTabIndex | Number | Tab index to be applied to all children of the list item
+init | Function() => void | Callback to initialize the list item on mount


I think its actually Callback executed when list item mounts

moog16 · 2018-10-23T22:02:43Z

test/unit/list/index.test.js

@@ -5,7 +5,7 @@ import {shallow, mount} from 'enzyme';
 import List from '../../../packages/list';
 import {ListItem} from '../../../packages/list';

-suite.only('List');
+suite('List');


add a test for removing a list item and adding a list item.

moog16 · 2018-10-23T22:04:21Z

packages/list/index.js

+    return this.getIndexOfListItemElement_(target);
+  }
+
+  onKeyDown = (e) => {


move onKeyDown, onClick, onFocus, and onBlur into the renderListItem, and pass the index. We can then remove getListItemIndexOfTarget_.

bonniezhou added 11 commits October 5, 2018 14:23

feat(list): Add component

db8db91

implement adapter methods

19db863

finish implementing list

2f2073e

list item

35d9c3c

add screenshot test

a035700

get selection and tabindex working

66523f4

get selection working

8fe7934

ListItem test

12ef3da

list tests

682f8b9

fix tests and lint

f26b9ac

README

112f19d

bonniezhou requested a review from moog16 October 16, 2018 01:11

Merge branch 'master' into feat/list

1013268

moog16 suggested changes Oct 16, 2018

View reviewed changes

name change

72e6cb2

moog16 self-assigned this Oct 17, 2018

bonniezhou added 6 commits October 17, 2018 19:59

break graphic text and meta to subcomponents

aa96143

fix nits for list

9d2a2a3

accept elements for text

7c55b77

merge classes instead of init

21c527d

condense screenshot tests

4eb1f3e

use keys instead of indices

706b8cb

moog16 suggested changes Oct 18, 2018

View reviewed changes

bonniezhou added 6 commits October 18, 2018 14:21

fix tests

5b526d4

fixes

ccfb8a8

more fixes

0017019

bug

a41652a

selection fix

167bd34

use id instead of key

d2726d2

bonniezhou and others added 9 commits October 20, 2018 15:40

list tests

38ff1c6

WIP tests

ca0a263

fix all tests

a9f5b84

readme

f057f83

Merge branch 'master' into feat/list

545a933

lint

39ac757

merge with master

c36e005

update screenshot golden

d40ae5b

only change list in screenshot goldens

2ffe55c

moog16 suggested changes Oct 23, 2018

View reviewed changes

moog16 reviewed Oct 23, 2018

View reviewed changes

bonniezhou and others added 2 commits October 23, 2018 15:29

fix readme

d8fc6c6

Merge branch 'master' into feat/list

fdc52fd

moog16 approved these changes Oct 23, 2018

View reviewed changes

fix layout grid screenshot test

585f340

bonniezhou merged commit 5ff3ecc into master Oct 24, 2018

bonniezhou deleted the feat/list branch October 24, 2018 16:35

bonniezhou mentioned this pull request Oct 24, 2018

fix(layout-grid): added golden to screenshots #368

Closed

liamcmitchell-sc mentioned this pull request Oct 24, 2019

ListItemGraphic, ListItemText, ListItemMeta have tabIndex -1 by default #1008

Closed

feat(list): Add component #339

feat(list): Add component #339

Uh oh!

Conversation

bonniezhou commented Oct 16, 2018

Uh oh!

moog16 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Oct 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

moog16 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moog16 Oct 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Oct 18, 2018 •

edited

Loading

moog16 Oct 18, 2018 •

edited

Loading