Adds support of List #212

darenju · 2016-02-10T18:00:13Z

Adds support of List, and List Item component. #201

Still thinking of how "primary content", "secondary content" & friends should be implemented. Would need help on that.

tleunen · 2016-02-10T18:16:35Z

I'd probably go with a specific primary content, and a secondary content to have everything flexible.
I'll take a deeper look later today and read the spec and mdl doc to see how we could do it.

darenju · 2016-02-10T22:11:15Z

So, we'd have a PrimaryContent and SecondaryContent components? Also, there's this difference between "secondary content" and "secondary actions", and the latter can be a link or a button, if I'm not mistaken.

tleunen · 2016-02-10T22:58:47Z

The ListPrimaryContent could take an icon and a subtitle as props, and children will be the title.
The ListSecondaryContent could take an info as prop and children will be the action.

I'm just throwing some ideas, I'd need to think about it. I'd like to provide an easy way to create a list of items (where the item is a link, like in the drawer for example), and the possibility to have that fully customizable :)

darenju · 2016-02-10T23:04:12Z

The ListPrimaryContent and ListSecondaryContent seem pretty decent.

However, I don't get your second paragraph. What we could do is providing a ListItemAction that would take a icon, a onClick and a child content for label. What do you think about this?

tleunen · 2016-02-10T23:18:47Z

Do you mean having List > ListItem > ListItemPrimary & ListItemSecondary > ListAction ? Or replacing Secondary with Action?
Could you write an example of jsx code? It will help me visualize it

darenju · 2016-02-10T23:40:14Z

For example, this code (3rd example on Components section of MDL):

<div class="demo-list-action mdl-list">
  <div class="mdl-list__item">
    <span class="mdl-list__item-primary-content">
      <i class="material-icons mdl-list__item-avatar">person</i>
      <span>Bryan Cranston</span>
    </span>
    <a class="mdl-list__item-secondary-action" href="#"><i class="material-icons">star</i></a>
  </div>
  <div class="mdl-list__item">
    <span class="mdl-list__item-primary-content">
      <i class="material-icons mdl-list__item-avatar">person</i>
      <span>Aaron Paul</span>
    </span>
    <a class="mdl-list__item-secondary-action" href="#"><i class="material-icons">star</i></a>
  </div>
  <div class="mdl-list__item">
    <span class="mdl-list__item-primary-content">
      <i class="material-icons mdl-list__item-avatar">person</i>
      <span>Bob Odenkirk</span>
    </span>
    <span class="mdl-list__item-secondary-content">
      <a class="mdl-list__item-secondary-action" href="#"><i class="material-icons">star</i></a>
  </span>
  </div>
</div>

Would be:

<List>
  <ListItem>
    <ListPrimaryContent icon="person">Bryan Cranston</ListPrimaryContent>
    <ListItemAction icon="star" onClick={/* ... */} />
  </ListItem>
  {/* ... */}
</List>

tleunen · 2016-02-11T00:44:10Z

Oh I see... The secondary content is always an action... Well.. the spec calls everything an action, but MDL only use action for the secondary content.

I won't put any event handler on that item, because it can be a checkbox or any other component...
So it could be <ListItemAction info="Actor"><Checkbox /></ListItemAction>

tleunen · 2016-02-11T01:00:51Z

I think we could rename everything though.. List, ListItem, ListItemContent and ListItemAction

I'm suggesting this:
List and ListItem are basic.
ListItemContent would be the primary content, can receive icon and subtitle as props. Subtitle is only used for 2 and 3 lines content, and for 3 lines, subtitle will use the body specific className, and children will be the main content.
ListItemAction is the secondary content, can take info as props (to display a small "header"), and children will be its content. I think it should also attach the className mdl-list__item-secondary-action on the child.

What do you think?

darenju · 2016-02-11T06:42:16Z

Everything seems clearer and precise now. Do we still agree that ListItem has twoLine and threeLine prop? Otherwise, we couldn't set .mdl-list__item--two-line or .mdl-list__item--three-line

If so, then it's alright. Attaching mdl-list__item--secondary-action to the child rather than on the parent seems actually more compliant to the specs.

If everything is alright, I could start writing it today.

tleunen · 2016-02-11T13:48:04Z

We can see this of 2 different ways I thing. Either the component takes a twoLines or threeLines prop. (or a single prop "lines" which takes 2 or 3?), or based on a subtitle or body prop, we can define if it's 2 or 3 lines. (subtitle = 2; body = 3).

I don't know if using subtitle and body is really good, since it can create confusion for the users. Using the number of lines is clear and you know exactly what you'll get.

darenju · 2016-02-11T13:59:45Z

Keeping the twoLine and threeLine (with trailing S or not?) is better than specifying body or subtitle as you mentioned it.

However, the point with CSS classes is that the parent must have it (-two-line or -three-line). Actually I found a way to pass this prop to ListItemContent if it's defined in ListItem.

I think we're all set, aren't we?

tleunen · 2016-02-11T14:07:46Z

Yep :-) Lets see some code and we'll see how it behaves :)

darenju · 2016-02-11T14:14:41Z

I'm currently working on it.

Have actually done a lot, and the trickiest part is over, I guess. Expect more commits by the end of the day. :)

darenju · 2016-02-11T15:35:42Z

I can't get to pass the Travis tests while it passes on my machine... Do you have any idea what it could be? @tleunen

tleunen · 2016-02-11T15:38:38Z

You can see the job task here https://travis-ci.org/tleunen/react-mdl/builds/108559317
Without investigating, I have no idea, but I think your tests don't need renderDOM, you could only use render instead and check the children with output.props.children

Please come on discord if you wanna discuss about it ;)

tleunen · 2016-02-11T16:44:01Z

src/List/ListItemAction.js

+
+        const children = React.Children.map(this.props.children, child => {
+            const toAdd = 'mdl-list__item-secondary-action';
+            const childClassName = child.props.className ? `${child.props.className} ${toAdd}` : toAdd;


you can use classNames('mdl-list__item-secondary-action', child.props.className) instead :)

tleunen · 2016-02-12T00:42:41Z

src/List/ListItem.js

+        const { className, twoLine, threeLine, ...otherProps } = this.props;
+
+        const classes = classNames('mdl-list__item', {
+            'mdl-list__item--two-line': twoLine,


twoLine && !threeLine.
same on the line below

(I'm still unsure about using a trailing s or not. Feels weird to not have it though :/)

tleunen · 2016-02-12T00:58:11Z

src/List/ListItemAction.js

+
+        let { children } = this.props;
+
+        try {


I'm not a big fan of try catch, especially in this case. The propType should be enough.

darenju · 2016-02-12T08:48:46Z

Thank you for your feedback! It helps me get better with React.

I have fixed most of the things, except the part for the icon/avatar point. We need to design this as well.

tleunen · 2016-02-12T15:00:23Z

src/List/List.js

@@ -0,0 +1,3 @@
+import basicClassCreator from '../utils/basicClassCreator';


You can move this in a src/List/index.js with the other imports as well. No need of a specific file since the end user will never import only this one (he won't be able to do much only with this)

darenju · 2016-02-12T18:16:01Z

Okay, tomorrow I'll work on avatar and everything else. Thanks for your feedback again.

tleunen · 2016-02-14T15:53:38Z

Any news on this? I can continue the work if needed

tleunen · 2016-02-14T21:18:15Z

Thank you @darenju for the work. I really would like to publish the new version today so I'll complete the work here with the said modifications and the documentation.

darenju · 2016-02-15T09:29:50Z

Sorry I haven't replied. I actually did some work, but was celebrating my birthday meanwhile. I'm glad my work has been merged. 👍

darenju added 2 commits February 10, 2016 17:17

Added List support

41f1171

Cleaned code

f550b2d

darenju added 2 commits February 11, 2016 15:10

Moved List component into separate directory

dd1b047

Added ListItem and ListItemContent with their tests

a2c3d5c

darenju added 3 commits February 11, 2016 15:52

Linter uses single quotes for string

49bfab8

Added ListItemAction and its tests

82bbad5

Fixed bad test and increased code coverage (only one return statement)

c2f5f4a

This damn test...

1a2faf6

tleunen reviewed Feb 11, 2016
View reviewed changes

darenju added 2 commits February 11, 2016 17:52

Fixed the test

b682e64

ListItemAction accepts only one child

8c35859

tleunen reviewed Feb 12, 2016
View reviewed changes

Fixed things after feedback

631caf2

tleunen reviewed Feb 12, 2016
View reviewed changes

tleunen closed this Feb 14, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support of List #212

Adds support of List #212

darenju commented Feb 10, 2016

tleunen commented Feb 10, 2016

darenju commented Feb 10, 2016

tleunen commented Feb 10, 2016

darenju commented Feb 10, 2016

tleunen commented Feb 10, 2016

darenju commented Feb 10, 2016

tleunen commented Feb 11, 2016

tleunen commented Feb 11, 2016

darenju commented Feb 11, 2016

tleunen commented Feb 11, 2016

darenju commented Feb 11, 2016

tleunen commented Feb 11, 2016

darenju commented Feb 11, 2016

darenju commented Feb 11, 2016

tleunen commented Feb 11, 2016

tleunen Feb 11, 2016

tleunen Feb 12, 2016

tleunen Feb 12, 2016

darenju commented Feb 12, 2016

tleunen Feb 12, 2016

darenju commented Feb 12, 2016

tleunen commented Feb 14, 2016

tleunen commented Feb 14, 2016

darenju commented Feb 15, 2016

		@@ -0,0 +1,3 @@
		import basicClassCreator from '../utils/basicClassCreator';

Adds support of List #212

Adds support of List #212

Conversation

darenju commented Feb 10, 2016

tleunen commented Feb 10, 2016

darenju commented Feb 10, 2016

tleunen commented Feb 10, 2016

darenju commented Feb 10, 2016

tleunen commented Feb 10, 2016

darenju commented Feb 10, 2016

tleunen commented Feb 11, 2016

tleunen commented Feb 11, 2016

darenju commented Feb 11, 2016

tleunen commented Feb 11, 2016

darenju commented Feb 11, 2016

tleunen commented Feb 11, 2016

darenju commented Feb 11, 2016

darenju commented Feb 11, 2016

tleunen commented Feb 11, 2016

tleunen Feb 11, 2016

Choose a reason for hiding this comment

tleunen Feb 12, 2016

Choose a reason for hiding this comment

tleunen Feb 12, 2016

Choose a reason for hiding this comment

darenju commented Feb 12, 2016

tleunen Feb 12, 2016

Choose a reason for hiding this comment

darenju commented Feb 12, 2016

tleunen commented Feb 14, 2016

tleunen commented Feb 14, 2016

darenju commented Feb 15, 2016