[List/ListItem] Introduce selected property #1581 #1976

frankf-cgn · 2015-10-23T14:29:39Z

Implementation of Issue #1581:

following the ReactLink pattern, List has now a selectedLink prop
selectedLink is a plain js object holding the index of the currently selected ListItem and a callback function to change that value
ListItem has new prop index
callback-fn of selectedLink is called when onTouchTap is fired by a ListItem
the original onTouchTap is propagated, too.

Example implementation for the container component for the List:

export default class MyListContainer extends Component {
  constructor(props) {
    super(props);
    this.state = { selectedIndex: 1 }
  }

  handleUpdateSelectedIndex = (index) => {
    this.setState(Object.assign({}, this.state, {selectedIndex: index}));
  }

  render() {
    var selectedLink = {
      value: this.state.selectedIndex,
      requestChange: this.handleUpdateSelectedIndex
    };
    return(
      <List style={styles.paper} 
        subheader="Contacts"
        selectedLink={selectedLink}>
        <ListItem index={1} ....../>
        <ListItem index={2} ....../>
      </List>
    );
  }
}

frankf-cgn · 2015-11-04T06:52:26Z

Is there anything I can do to get this merged?

oliviertassinari · 2015-11-04T10:46:02Z

src/lists/list-item.jsx


    const styles = {
      root: {
-        backgroundColor: (this.state.isKeyboardFocused || this.state.hovered) &&
+        backgroundColor: (this.props.selected && !this.state.hovered) ? selectedColor : ((this.state.isKeyboardFocused || this.state.hovered) &&


the ...other is going to have the selected property then passed to the EnhancedButton. Is this intentional? Otherwise, can you remove it.

frankf-cgn · 2015-11-04T11:17:51Z

I've set the default value as suggested and fixed the unintentionally passing of selected to the EnhancedButton.

oliviertassinari · 2015-11-04T11:41:27Z

src/lists/list.jsx

@@ -85,16 +88,39 @@ const List = React.createClass({
      subheaderElement = <div style={mergedSubheaderStyles}>{subheader}</div>;
    }

+    let listItems = React.Children.map(children, (child) => {
+      if ( this.props.selectedLink && (child.type.displayName === "ListItem") ) {


I would move if(this.props.selectedLink out of the loop, for perf reasons. What do you think?

I fully agree.

oliviertassinari · 2015-11-04T11:46:33Z

@frankf-cgn Thanks. Could you have a look at my feedbacks? I think that this PR is going in the right direction 👍. I will also ask you to squash down the number of commit (to 2 or 3). I will keep the history clean.

frankf-cgn · 2015-11-04T13:24:55Z

@oliviertassinari Thanks for your review and time. I fixed the loop and I'll do the documentation stuff. And of cource I'm going to shrink down the number of commits.

frankf-cgn · 2015-11-04T16:04:34Z

@oliviertassinari I have fixed the loop and added the documentation. Concerning the documentation I have tried to choose the wording to be close to the tabs-stuff cos it's somehwat similar to this. Let me know if you are fine with it and I'll clean up the commit history and rebase to the current master.

frankf-cgn · 2015-11-05T09:00:39Z

@oliviertassinari Squashed the commits

frankf-cgn · 2015-11-07T10:42:35Z

@oliviertassinari If you are ready to merge this, come back to me and I will rebase to the actual master.

oliviertassinari · 2015-11-07T11:20:01Z

docs/src/app/components/pages/components/lists.jsx

+  }
+
+  handleUpdateSelectedIndex = (index) => {
+    this.setState(Object.assign({}, this.state, {selectedIndex: index}));


Why not doing?

this.setState({ selectedIndex: index, });

The way I proposed to set the state ensures that whatever is currently in the state does not get overwritten / thrown away unintentionally, isn't it?

As far as I know, we have the same result with the above. See the setState doc:

Performs a shallow merge of nextState into current state

Uups, didn't rember that. So I'll change it.

frankf-cgn · 2015-11-07T12:07:31Z

@oliviertassinari Thanks for the hints. I have incorporated your suggestions.
One question. In list-item.jsx around line 102, should i add index to const {..., ...other}, so that index is not further propagated to EnhancedButton like you suggested for selected?

oliviertassinari · 2015-11-07T12:30:06Z

should i add index to const {..., ...other}, so that index is not further propagated to EnhancedButton like you suggested for selected?

Sounds like a good idea

shaurya947 · 2015-11-10T22:00:55Z

@oliviertassinari OK to merge?

oliviertassinari · 2015-11-10T22:11:09Z

@shaurya947 I didn't try it but it looks good to me.

oliviertassinari · 2015-11-10T22:13:36Z

src/lists/list.jsx

      </Paper>
    );
  },
+
+  _getSelected(index) {


Not sure we need to create two methods since they are called at only one place.

shaurya947 · 2015-11-10T22:26:30Z

@oliviertassinari I think you should check it out before you merge since you've reviewed the code (and are more familiar with it).

oliviertassinari · 2015-11-10T22:30:39Z

before you merge

I wasn't going to merge yet 😁

shaurya947 · 2015-11-10T22:43:44Z

Well, eventually, right? :-P

shaurya947 · 2015-11-16T18:31:44Z

docs/src/app/components/pages/components/lists.jsx

@@ -54,6 +85,12 @@ export default class ListsPage extends React.Component {
            desc: 'If true, the subheader will be indented by 72px.',
          },
          {
+            name: 'selectedItemStyle',
+            type: 'object',
+            header: 'optional, only available it HOC SelectableContainerEnhance is used',


only available if HOC

shaurya947 · 2015-11-16T19:02:27Z

@frankf-cgn thus far we've used simple html tags with some CodeBlock components for documentation. I think we should try to stick to that for consistency and avoid the whole markdown thing. See here for an example.

frankf-cgn · 2015-11-17T16:55:33Z

The typos are fixed, some rewording has taken place and I have removed the markdown stuff. Hope we have reached the last itterations ;-)

shaurya947 · 2015-11-17T18:32:32Z

docs/src/app/components/pages/components/lists.jsx

+          <ul>
+            <li> enhance <code>&lt;List&gt;</code> with HOC</li>
+            <li> decide where to put state</li>
+            <li> implement end set valueLink</li>


should this read "implement and set valueLink" ?

shaurya947 · 2015-11-17T18:40:16Z

I just pointed out a few quick things @frankf-cgn. Once you can take care of them, we should merge this.

I'll also give @oliviertassinari a chance to look over this.

frankf-cgn · 2015-11-17T19:30:26Z

@shaurya947 I was so sure that I have find all typos during my proofreading. Ok, everthing done by now and pushed.
I have removed this nasty /* eslint-disable */which effectivly disabled eslint for the code.

oliviertassinari · 2015-11-17T20:13:51Z

@shaurya947 Let's see

oliviertassinari · 2015-11-17T20:17:32Z

docs/src/app/components/pages/components/lists.jsx

+          </p>
+          <div style={styles.codeblock}>
+            <CodeBlock>
+{`import { SelectableContainerEnhance } from 'material-ui/hoc/selectable-enhance';


missing lib/?

[List/ListItem] Introduce selected property #1581

oliviertassinari · 2015-11-18T11:02:37Z

@frankf-cgn Thank you!

frankf-cgn · 2015-11-18T11:07:56Z

@oliviertassinari @shaurya947 I have to thank you both!

shaurya947 · 2015-11-18T16:45:54Z

Yay! Great job @frankf-cgn

…ui#1976)

oliviertassinari added the GH Review: review-needed label Oct 26, 2015

oliviertassinari reviewed Nov 4, 2015
View reviewed changes

oliviertassinari added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed Review: review-needed labels Nov 4, 2015

oliviertassinari added Review: review-needed and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Nov 4, 2015

frankf-cgn force-pushed the issue/1581 branch from c475b9b to 9782ec6 Compare November 5, 2015 08:57

frankf-cgn force-pushed the issue/1581 branch from 9782ec6 to 006a180 Compare November 6, 2015 10:25

oliviertassinari reviewed Nov 7, 2015
View reviewed changes

frankf-cgn force-pushed the issue/1581 branch from 006a180 to 4e1d559 Compare November 7, 2015 13:04

oliviertassinari closed this Nov 8, 2015

oliviertassinari reopened this Nov 8, 2015

oliviertassinari reviewed Nov 10, 2015
View reviewed changes

shaurya947 reviewed Nov 16, 2015
View reviewed changes

[List] Implement SelectableList using a HOC

615c314

frankf-cgn force-pushed the issue/1581 branch 2 times, most recently from ec3b30e to 1e04820 Compare November 17, 2015 11:21

shaurya947 reviewed Nov 17, 2015
View reviewed changes

frankf-cgn force-pushed the issue/1581 branch from 1e04820 to a47a59e Compare November 17, 2015 19:27

oliviertassinari reviewed Nov 17, 2015
View reviewed changes

shaurya947 mentioned this pull request Nov 17, 2015

[ListItem] onTouchStart is not invoked, onTouchTap is missing #1099

Closed

[List] Add documentation for HOC selectable-enhance

f76b83b

frankf-cgn force-pushed the issue/1581 branch from a47a59e to f76b83b Compare November 18, 2015 10:55

oliviertassinari added a commit that referenced this pull request Nov 18, 2015

Merge pull request #1976 from frankf-cgn/issue/1581

e1aed16

[List/ListItem] Introduce selected property #1581

oliviertassinari merged commit e1aed16 into mui:master Nov 18, 2015

frankf-cgn deleted the issue/1581 branch November 20, 2015 07:23

alitaheri removed the PR: Needs Review label Feb 15, 2016

mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Nov 10, 2020

[core] Add --report-unused-disable-directives to lint commands on CI (m…

d701a81

…ui#1976)

zannager added the component: list This is the name of the generic UI component, not the React module! label Mar 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[List/ListItem] Introduce selected property #1581 #1976

[List/ListItem] Introduce selected property #1581 #1976

frankf-cgn commented Oct 23, 2015

frankf-cgn commented Nov 4, 2015

oliviertassinari Nov 4, 2015

frankf-cgn commented Nov 4, 2015

oliviertassinari Nov 4, 2015

frankf-cgn Nov 4, 2015

oliviertassinari commented Nov 4, 2015

frankf-cgn commented Nov 4, 2015

frankf-cgn commented Nov 4, 2015

frankf-cgn commented Nov 5, 2015

frankf-cgn commented Nov 7, 2015

oliviertassinari Nov 7, 2015

frankf-cgn Nov 7, 2015

oliviertassinari Nov 7, 2015

frankf-cgn Nov 7, 2015

frankf-cgn commented Nov 7, 2015

oliviertassinari commented Nov 7, 2015

shaurya947 commented Nov 10, 2015

oliviertassinari commented Nov 10, 2015

oliviertassinari Nov 10, 2015

shaurya947 commented Nov 10, 2015

oliviertassinari commented Nov 10, 2015

shaurya947 commented Nov 10, 2015

shaurya947 Nov 16, 2015

shaurya947 commented Nov 16, 2015

frankf-cgn commented Nov 17, 2015

shaurya947 Nov 17, 2015

shaurya947 commented Nov 17, 2015

frankf-cgn commented Nov 17, 2015

oliviertassinari commented Nov 17, 2015

oliviertassinari Nov 17, 2015

oliviertassinari commented Nov 18, 2015

frankf-cgn commented Nov 18, 2015

shaurya947 commented Nov 18, 2015

[List/ListItem] Introduce selected property #1581 #1976

[List/ListItem] Introduce selected property #1581 #1976

Conversation

frankf-cgn commented Oct 23, 2015

frankf-cgn commented Nov 4, 2015

Choose a reason for hiding this comment

frankf-cgn commented Nov 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Nov 4, 2015

frankf-cgn commented Nov 4, 2015

frankf-cgn commented Nov 4, 2015

frankf-cgn commented Nov 5, 2015

frankf-cgn commented Nov 7, 2015

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

frankf-cgn commented Nov 7, 2015

oliviertassinari commented Nov 7, 2015

shaurya947 commented Nov 10, 2015

oliviertassinari commented Nov 10, 2015

Choose a reason for hiding this comment

shaurya947 commented Nov 10, 2015

oliviertassinari commented Nov 10, 2015

shaurya947 commented Nov 10, 2015

Choose a reason for hiding this comment

shaurya947 commented Nov 16, 2015

frankf-cgn commented Nov 17, 2015

Choose a reason for hiding this comment

shaurya947 commented Nov 17, 2015

frankf-cgn commented Nov 17, 2015

oliviertassinari commented Nov 17, 2015

Choose a reason for hiding this comment

oliviertassinari commented Nov 18, 2015

frankf-cgn commented Nov 18, 2015

shaurya947 commented Nov 18, 2015