Skip to content

Fixed select all behavior across Data views #7200

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 17 commits into from
Apr 12, 2024
Merged

Fixed select all behavior across Data views #7200

merged 17 commits into from
Apr 12, 2024

Conversation

halocline
Copy link
Collaborator

What does this PR do?

Closes #7199 by persisting selected records across various Data views.

Where should the reviewer start?

DataTable/Header.js

What testing has been done on this PR?

Using the story Data/OnSelect

Select all & unselect all across a variety of views, altering current page, filters, search, etc.

How should this be manually tested?

Using the story Data/OnSelect

Do Jest tests follow these best practices?

  • screen is used for querying.
  • The correct query is used. (Refer to this list of queries)
  • asFragment() is used for snapshot testing.

Any background context you want to provide?

What are the relevant issues?

#7199

Screenshots (if appropriate)

Do the grommet docs need to be updated?

No.

Should this PR be mentioned in the release notes?

Yes.

Is this change backwards compatible or is it a breaking change?

Backwards compatible.

@halocline halocline requested review from taysea and britt6612 April 11, 2024 19:43
taysea
taysea reviewed Apr 11, 2024
View reviewed changes
@halocline
DCO Remediation Commit for Matt Glissmann <mdglissmann@gmail.com>
8d9c004 
I, Matt Glissmann <mdglissmann@gmail.com>, hereby add my Signed-off-by to this commit: eca0c68
I, Matt Glissmann <mdglissmann@gmail.com>, hereby add my Signed-off-by to this commit: e5be44a
I, Matt Glissmann <mdglissmann@gmail.com>, hereby add my Signed-off-by to this commit: ee25d24
I, Matt Glissmann <mdglissmann@gmail.com>, hereby add my Signed-off-by to this commit: 9698311
I, Matt Glissmann <mdglissmann@gmail.com>, hereby add my Signed-off-by to this commit: 42b1c24
I, Matt Glissmann <mdglissmann@gmail.com>, hereby add my Signed-off-by to this commit: 557c5f3
I, Matt Glissmann <mdglissmann@gmail.com>, hereby add my Signed-off-by to this commit: 3ab027a
I, Matt Glissmann <mdglissmann@gmail.com>, hereby add my Signed-off-by to this commit: ddf4052

Signed-off-by: Matt Glissmann <mdglissmann@gmail.com>
taysea
taysea reviewed Apr 11, 2024
View reviewed changes
taysea
taysea reviewed Apr 11, 2024
View reviewed changes
taysea
taysea reviewed Apr 11, 2024
View reviewed changes
taysea
taysea reviewed Apr 11, 2024
View reviewed changes
taysea
taysea reviewed Apr 11, 2024
View reviewed changes
taysea
taysea reviewed Apr 11, 2024
View reviewed changes
@halocline
simplify condition and remove comments
5457ec8 
Signed-off-by: Matt Glissmann <mdglissmann@gmail.com>
@halocline halocline requested a review from taysea April 11, 2024 22:46
This was referenced Apr 12, 2024
Closed
Merged
@britt6612
Copy link
Collaborator

britt6612 commented Apr 12, 2024 •
edited
Loading

One thing I was looking at what if we have 1 selected but disabled

is this the correct behavior? As you can see the Amos-17 is selected and disabled Is this still the correct behavior for the intermediate or would we expect this to be checked?

Screen Shot 2024-04-12 at 2 18 34 PM

The count seems off
when nothing is selected besides the one that is disabled and selected it shows one however when I select everything is shows one less.

Screen Shot 2024-04-12 at 2 20 15 PM

@halocline
Copy link
Collaborator Author

halocline commented Apr 12, 2024 •
edited
Loading

One thing I was looking at what if we have 1 selected but disabled

is this the correct behavior? As you can see the Amos-17 is selected and disabled Is this still the correct behavior for the intermediate or would we expect this to be checked?


The count seems off when nothing is selected besides the one that is disabled and selected it shows one however when I select everything is shows one less.

No. You are correct, we would expect the selected count to equal the total records.

@halocline
Copy link
Collaborator Author

One thing I was looking at what if we have 1 selected but disabled
is this the correct behavior? As you can see the Amos-17 is selected and disabled Is this still the correct behavior for the intermediate or would we expect this to be checked?

The count seems off when nothing is selected besides the one that is disabled and selected it shows one however when I select everything is shows one less.

No. You are correct, we would expect the selected count to equal the total records.

@britt6612 - Actually, I believe it is behaving correctly. The story had multiple items disabled which is why when 'Amos-17' was selected and disabled, 'CRS-21' was still just disabled. ('CRS-22' doesn't actually exist in the data set).

const disabled = ['Amos-17', 'CRS-21', 'CRS-22'];

I'm going to remove the 'disabled' from the story after you complete your review. It's helpful for QA but not for the story itself.

@britt6612
Copy link
Collaborator

britt6612 commented Apr 12, 2024 •
edited
Loading

One thing I was looking at what if we have 1 selected but disabled
is this the correct behavior? As you can see the Amos-17 is selected and disabled Is this still the correct behavior for the intermediate or would we expect this to be checked?
The count seems off when nothing is selected besides the one that is disabled and selected it shows one however when I select everything is shows one less.

No. You are correct, we would expect the selected count to equal the total records.

@britt6612 - Actually, I believe it is behaving correctly. The story had multiple items disabled which is why when 'Amos-17' was selected and disabled, 'CRS-21' was still just disabled. ('CRS-22' doesn't actually exist in the data set).

const disabled = ['Amos-17', 'CRS-21', 'CRS-22'];

I'm going to remove the 'disabled' from the story after you complete your review. It's helpful for QA but not for the story itself. since

your correct the 'CRS-21' was disabled that was missing from count

@halocline
add groupBy.select handling
94bcfb4 
Signed-off-by: Matt Glissmann <mdglissmann@gmail.com>
@halocline
Merge branch 'master' into data-onselect
c951abf
@halocline
DCO Remediation Commit for Taylor Seamans <taylor.seamans@yahoo.com>
c6f65d2 
I, Taylor Seamans <taylor.seamans@yahoo.com>, hereby add my Signed-off-by to this commit: 835893f

Signed-off-by: Taylor Seamans <taylor.seamans@yahoo.com>
Signed-off-by: Matt Glissmann <mdglissmann@gmail.com>
taysea
taysea approved these changes Apr 12, 2024
View reviewed changes
Copy link
Collaborator

@taysea taysea left a comment

Choose a reason for hiding this comment

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

LGTM!

@taysea taysea requested a review from MikeKingdom April 12, 2024 22:19
@taysea taysea merged commit 89050e0 into master Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taysea taysea taysea approved these changes

@britt6612 britt6612 Awaiting requested review from britt6612

@MikeKingdom MikeKingdom Awaiting requested review from MikeKingdom

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Data table selections should persist across DataViews
3 participants
@halocline @britt6612 @taysea