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] Fixing selectAll functionality a bit #4589

Closed
wants to merge 1 commit into from

Conversation

cody-lettau
Copy link

@cody-lettau cody-lettau commented Jun 30, 2016

This PR is for improvements made to the Table component's functionality. There are quite a few issues that basically render the component unusable if you were to want to use the selectable property. Although this PR does not fix every issue out there, it does appear to fix #1345, as well as improves handling of selecting/unselecting rows.

For example, when using select all, it would not uncheck all rows after unselecting the check all box (when 'deselect on clickaway' is disabled).

Another example is when using select all and you have all the boxes checked and then click on one, it will change to only have that single one selected. Currently, when you have rows that were preselected (like the complex example), it incorrectly leaves the wrong one checked (if you try checking one of the preselected rows).

Basically, my goal is to get this component to a point where we can use it in our project. The way it stands right now with how the select all functionality works, we'd have to do some hacking to get it working. This PR should fix the major issues with that functionality.

  • PR has tests / docs demo, and is linted.
  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
  • Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

Closes #1345

@@ -142,6 +142,14 @@ class Table extends Component {
allRowsSelected: this.props.allRowsSelected,
};

componentWillReceiveProps(nextProps) {
if (typeof nextProps.allRowsSelected !== 'undefined' && this.props.allRowsSelected !== nextProps.allRowsSelected) {
Copy link
Author

Choose a reason for hiding this comment

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

Here we are allowing for the maintaining of allRowsSelected from outside of the component. This is necessary if utilizing "selected" on the data.

When maintaining the "selected" state outside of the table, you should expect to have to maintain the "all selected" state as well I believe.

@cody-lettau
Copy link
Author

Note that when deselectOnClickaway is disabled, this PR also fixes the issue with unchecking the select all checkbox and one of the rows staying checked.

@muibot muibot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 15, 2016
@oliviertassinari
Copy link
Member

The Table component was partially / fully migrated on the next branch.
We are now focusing on releasing out the next branch.
We plan to stop the support of the migrated components.
I'm gonna close this PR. Thanks for your time.
Would be great if you check out the new component. Any PR is welcomed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants