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] Fix TableBody selectedRows state #5829

Merged
merged 2 commits into from
Dec 28, 2016

Conversation

ovaldi
Copy link

@ovaldi ovaldi commented Dec 27, 2016

I have a bug when I "Unselect All" rows. (1. click select all to select all rows 2. click select all again to unselect all rows).

Here is the TableBody.js source code:

      this.setState({
        selectedRows: this.state.selectedRows.length > 0 ?
          [this.state.selectedRows[this.state.selectedRows.length - 1]] : [],
      });

Basically, this line of code evaluates to "If I have any selected rows, let my new selected row be the last selected rows)"

eg.

  1. If I have 3 items in a grid and all of them are unselected:

□ 1 Dog
□ 2 Cat
□ 3 Rat

  1. When I click on the Select All button to select all selections:

√ 1 Dog
√ 2 Cat
√ 3 Rat

  1. When I click on the Select All button to remove all selections, I will end up with the following result:

□ 1 Dog
□ 2 Cat
√ 3 Rat

The last row still selected, in this case, I just want to unselect all rows.

Closes #5249.
Closes #5192.
Closes #5234.

@muibot muibot added PR: Needs Review 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 PR: Needs Review labels Dec 27, 2016
@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 27, 2016

You are the third person asking for that change. Let's move into that direction. Could you write a unit test with the above issue description?
That would help a lot improving the state management. Thanks!

@oliviertassinari oliviertassinari added component: table This is the name of the generic UI component, not the React module! and removed PR: Needs Review labels Dec 27, 2016
@oliviertassinari oliviertassinari changed the title fix TableBody selectedRows [Table] fix TableBody selectedRows state Dec 27, 2016
@oliviertassinari oliviertassinari added PR: Review Accepted 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 Dec 28, 2016
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm gonna handle the tests myself. Thanks for the PR.

@oliviertassinari oliviertassinari merged commit 824a8f2 into mui:master Dec 28, 2016
@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Dec 28, 2016
@ovaldi
Copy link
Author

ovaldi commented Dec 29, 2016

Thank you @oliviertassinari , :)

@oliviertassinari oliviertassinari changed the title [Table] fix TableBody selectedRows state [Table] Fix TableBody selectedRows state Jan 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: table This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants