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

Added support for Shift + Click selection of rows #827

Merged
merged 7 commits into from
Aug 29, 2019
Merged

Added support for Shift + Click selection of rows #827

merged 7 commits into from
Aug 29, 2019

Conversation

patorjk
Copy link
Collaborator

@patorjk patorjk commented Aug 11, 2019

An implementation of the feature suggested in #663

The UX for shift+click is modeled off of how GMail does it. For example, if you select/unselect row A and then hold shift and select/unselect row B:

  • If row B is being selected, then the rows between A and B (including A) will be selected.
  • If row B is being unselected, then the rows between A and B (including A) will be unselected.

If any filters are removed or columns resorted or selection headers toggled or data fetched (in the case of Server Side tables), between the selection of rows A and B, then there will be no special effect.

@coveralls
Copy link

coveralls commented Aug 11, 2019

Coverage Status

Coverage increased (+1.03%) to 76.248% when pulling 7581dab on patorjk:shift-click into d1914e4 on gregnb:master.

@patorjk
Copy link
Collaborator Author

patorjk commented Aug 11, 2019

This PR is most likely not compatible with #825, however, I could adjust it so that in the MUIDataTable.js file, when it was in the process of adding the adjacent rows, it would stop if it hit the maxSelectedRows threshold.

@gabrielliwerant gabrielliwerant self-requested a review August 12, 2019 05:26
@gabrielliwerant gabrielliwerant added the needs review Useful to mark PRs for what's up next to review label Aug 12, 2019
@gabrielliwerant
Copy link
Collaborator

This is a cool feature, thank you for working on this!

I just reviewed and suggested a number of changes to #825, so how about we wait for that to be merged and integrated with yours before review?

@patorjk
Copy link
Collaborator Author

patorjk commented Aug 15, 2019

Sounds good, I’ll be unavailable next week but could make any adjustments tomorrow or the week after that.

@gabrielliwerant gabrielliwerant mentioned this pull request Aug 22, 2019
@gabrielliwerant
Copy link
Collaborator

@patorjk The isRowSelectable changes are in latest master now, if you want to pull those.

@gabrielliwerant gabrielliwerant added needs work and removed needs review Useful to mark PRs for what's up next to review on hold labels Aug 22, 2019
@patorjk
Copy link
Collaborator Author

patorjk commented Aug 23, 2019

Sounds good, I’ll pull them and update this PR early next week!

@patorjk
Copy link
Collaborator Author

patorjk commented Aug 27, 2019

For trying this out, I added (but didn't commit) an isRowSelectable function that looked like this:

  isRowSelectable: function(dataIndex, selectedRows) {
    //return true;
    if (selectedRows.data.length > 4 && selectedRows.data.filter(d => d.dataIndex === dataIndex).length === 0) return false;
    return data[dataIndex][1] != "Attorney";
  }

@gabrielliwerant gabrielliwerant added needs review Useful to mark PRs for what's up next to review and removed needs work labels Aug 28, 2019
Copy link
Collaborator

@gabrielliwerant gabrielliwerant left a comment

Choose a reason for hiding this comment

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

Worked really well for me! I tested in multiple scenarios, including the isSelectable you pointed out. I only have a few nitpicks, but otherwise looks great.


// Create a copy of the selectedRows object. This will be used and modified
// below when we see if we can add adjacent rows.
let selectedRows = JSON.parse(JSON.stringify(this.props.selectedRows));
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be self-documenting and probably a bit less expensive to use lodash.clonedeep, which is in the library, here instead.

let selectedRows = JSON.parse(JSON.stringify(this.props.selectedRows));

// Add the clicked on row to our copy of selectedRows (if it isn't already present).
var clickedDataIndex = this.props.data[data.index].dataIndex;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe we can use let here and below on line 129 since the variables don't need to leave their respective blocks.

@gabrielliwerant gabrielliwerant added needs work and removed needs review Useful to mark PRs for what's up next to review labels Aug 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants