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

Clear list selections on pagination/search/sort #10329

Merged

Conversation

keithjgrant
Copy link
Member

SUMMARY

Updates nearly every list* so that URL param changes (pagination, search, or sort) clear the selection. This prevents the list of selected items in state from including items that may no longer appear on screen — preventing the user from accidentally deleting or otherwise altering an item they may not realize they still have selected.

This also updates the useSelected hook to provide selectAll and clearSelected functions. Any lists that weren't yet already using this hook have been updated to do so.

Addresses #6853 and #7509

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME
  • UI
ADDITIONAL INFORMATION

*Lists that do not include this change are modals where the user is expected to paginate through screens and make several selections along the way (e.g. Multi Credential select modal), and lists that still use PaginatedDataList and are yet to be converted to PaginatedTable

Note: I originally wanted to make the clearSelected prop on PaginatedTable required, so any lists that don't have this fix applied would fail loudly. Unfortunately that wasn't possible, as there were a few lists that should not have this behavior, so I had to leave it as an optional prop.

@softwarefactory-project-zuul
Copy link
Contributor

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.

Copy link
Member

@AlexSCorey AlexSCorey left a comment

Choose a reason for hiding this comment

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

I think this is a really easy and clean way of solving this problem. We probably want to write some tests for the unselecting of list items on search, pagination etc, at the very least, inside useSelected.test.jsx

Copy link
Contributor

@jakemcdermott jakemcdermott left a comment

Choose a reason for hiding this comment

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

👍

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@akus062381 akus062381 self-assigned this Jun 2, 2021
@akus062381
Copy link
Member

@keithjgrant this looks good!

@softwarefactory-project-zuul
Copy link
Contributor

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.

@akus062381 akus062381 removed the mergeit label Jun 2, 2021
@akus062381
Copy link
Member

@keithjgrant can you please rebase and fix the conflict? once you are able to do that, we can add the mergeit label

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

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.

5 participants