Skip to content

Conversation

@manuel-delvillar
Copy link
Contributor

To reproduce this in local

  1. Go to any collection
  2. Click on "Add RSDs to This Collection"
  3. Do a search (we need more results than the size of the pagination)
  4. Try to add only all in the current page

@manuel-delvillar manuel-delvillar self-assigned this Apr 27, 2023
@manuel-delvillar manuel-delvillar added the bug Something isn't working label Apr 27, 2023
@manuel-delvillar manuel-delvillar marked this pull request as ready for review April 27, 2023 23:39
@ottonomy
Copy link
Contributor

ottonomy commented Apr 28, 2023

Let me see if iI'm correctly summarizing some issues here. It looks like prior to your fix:

  • the select all control and the paginated list (where you are only seeing part of the result set) have not been properly designed to work together.
  • This results in symptom: When you select all on one page, and then go forward a page, none appear selected, but the select all box remains checked.
  • The count next to the select all box, e.g. "Select All (108)", seemed to apply across all pages.
  • When 3 results on page 1 are selected, advance to page 2, then back to page 1, none appear selected on page 1, so it's not possible for the user to submit something across pages.

Some stuff that happens when requests are submitted:

If "select all (108)" is selected, what gets requested to the server is actually a query-based change order:

{
    "add": {
        "query": "a"
    }
}

But if you then go back and select individual items and submit again without refreshing, the same thing is sent again, there might be some bugs here to investigate.

After refreshing and selecting only two skills, I got a different payload sent to server

{
    "add": {
        "uuids": [
            "21901b21-233e-45b7-be5d-8fbf93ea5aeb",
            "75b895ac-7a47-4113-a43e-b9a3d14a977b"
        ]
    }
}

So, the UX needs to be made consistent. The user needs to understand that the select all control is different from the list of checkboxes in the table in purpose. And importantly, that means that when the "select all (108)" box is selected, the in-table controls should be checked disabled.

If then you wanted to add a separate "select page (50)" control (disabled when "select all (108)" is selected though), that would be a fine complement.

Ok, so the new UX in this PR:

  • now there is the ability to change what form of selection is occurring. The select all checkbox becomes a "select all " checkbox combined with a "" select box.
  • The user can "select all (page)" or "select all (108)" to control what the checkbox does.
  • if the user has select all checkbox checked, then it doesn't matter what they check or uncheck in the table. This is confusing. If those controls don't have an effect, they should be disabled but still clear as to whether or not that RSD will be submitted.

Screenshot 2023-04-27 at 5 16 38 PM

Recommendation: If you just set all the row level checkboxes to be disabled when the select all box at the top is selected, then this checks all the boxes for good UX I think.

A11y recommendation: I recommend testing the keyboard nav and screen reader experience + keyboard nav experience. The "select " control doesn't have the right visual focus indicated when keyboard navigating.

@JohnKallies JohnKallies requested a review from a team May 1, 2023 18:32
@Corpratespaz Corpratespaz force-pushed the fix/select-all-select-page branch from a0bfb9d to 984b009 Compare May 15, 2023 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Development

Successfully merging this pull request may close these issues.

3 participants