Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Table] Demo for table should derive the sorted data #11827

Closed
2 tasks done
charlax opened this issue Jun 12, 2018 · 1 comment
Closed
2 tasks done

[Table] Demo for table should derive the sorted data #11827

charlax opened this issue Jun 12, 2018 · 1 comment
Labels
component: table This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation new feature New feature or request

Comments

@charlax
Copy link
Contributor

charlax commented Jun 12, 2018

  • This is a v1.x issue (v0.x is no longer maintained).
  • I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior

The Table "Sorting & Selecting" component demo should make it easy to follow React best practices. So, if I quickly change data for this.props.data, it should just work:

class EnhancedTable extends React.Component {
  constructor(props) {
    super(props);

    this.state = {
      order: 'asc',
      orderBy: 'calories',
      selected: [],
//  ⚠️ NOTE: data is stored in the state
      data: props.data,
      ...

Current Behavior

Used as is, the EnhancedTable component will not rerender when props change. The React documentation warns against copying props into the state (see here). Yet with this example it is very easy to replace the static data with props.data and expect it to work as is.

Steps to Reproduce (for bugs)

I'd be happy to provide steps to reproduce, not sure that's necessary here since there's a better pattern for the example that is fairly easy to implement.

The problem can be found in the EnhancedTable constructor:

class EnhancedTable extends React.Component {
  constructor(props) {
    super(props);

    this.state = {
      order: 'asc',
      orderBy: 'calories',
      selected: [],
//  ⚠️ NOTE: data is stored in the state
      data: [
        createData('Cupcake', 305, 3.7, 67, 4.3),
        createData('Donut', 452, 25.0, 51, 4.9),
        createData('Eclair', 262, 16.0, 24, 6.0),
        createData('Frozen yoghurt', 159, 6.0, 24, 4.0),
        createData('Gingerbread', 356, 16.0, 49, 3.9),
        createData('Honeycomb', 408, 3.2, 87, 6.5),
        createData('Ice cream sandwich', 237, 9.0, 37, 4.3),
        createData('Jelly Bean', 375, 0.0, 94, 0.0),
        createData('KitKat', 518, 26.0, 65, 7.0),
        createData('Lollipop', 392, 0.2, 98, 0.0),
        createData('Marshmallow', 318, 0, 81, 2.0),
        createData('Nougat', 360, 19.0, 9, 37.0),
        createData('Oreo', 437, 18.0, 63, 4.0),
      ].sort((a, b) => (a.calories < b.calories ? -1 : 1)),
      page: 0,
      rowsPerPage: 5,
    };
  }

handleRequestSort = (event, property) => {
    const orderBy = property;
    let order = 'desc';

    if (this.state.orderBy === property && this.state.order === 'desc') {
      order = 'asc';
    }

//  ⚠️ NOTE: on sorting change, `state.data` is reset
    const data =
      order === 'desc'
        ? this.state.data.sort((a, b) => (b[orderBy] < a[orderBy] ? -1 : 1))
        : this.state.data.sort((a, b) => (a[orderBy] < b[orderBy] ? -1 : 1));

    this.setState({ data, order, orderBy });
  };

I think the example would be simpler and more correct if the sorted data was computed when rendering. I will be providing a PR with a potential solution.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 12, 2018

@charlax I think that storing the sorted data into the state is done in order to improve performance. I think that it's something I would keep for an application running in production, at least, if I had a significant amount of data and repetitive renderings.

However, for the sack of this demo, I agree, we can move toward a more "pure" logic (with less state) 👍 .

@oliviertassinari oliviertassinari added new feature New feature or request docs Improvements or additions to the documentation component: table This is the name of the generic UI component, not the React module! labels Jun 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: table This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation new feature New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants