kolibri icon indicating copy to clipboard operation
kolibri copied to clipboard

Update UserTable to allow single selection

Open MisRob opened this issue 2 years ago • 1 comments

Summary

  • Adds a basic test suite for UserTable, particularly for areas that are later updated by this pull request
  • Adds a new property, enableMultipleSelection defaulting to true. When the table is selectable and enableMultipleSelection is true, checkboxes will be displayed for user selection which covers all our previous use cases of the selectable table.
  • Implements radio buttons for a single user selection when enableMultipleSelection is false. This is to be used on the choose admin page of the change facility flow (not included in this pull request):
  • Related accessibility improvements of the selectable table
    • Moved the Select all checkbox outside of the first table column header and used CSS to position it instead. This allowed this <th> to be used for describing what that table column contains instead, by means of the Select user by: visually hidden text that precedes the Full name text
    • Used users' full names as labels of checkboxes/radio buttons (previously, all checkboxes had the same Select user visually hidden label which couldn't communicate well what user is being selected)
  • Moves the table to core components since it's used by more plugins

References

Related https://github.com/learningequality/kolibri/issues/9326

Opened related KDS issues:

  • https://github.com/learningequality/kolibri-design-system/issues/347

Reviewer guidance

I wanted to implement radio button selection in an accessible way. Doing it only for radio buttons without making any changes to checkboxes would be complicated since the previous and new solution has different table columns structure. Because there's lots of refactoring in that regard, I'm opening a separate pull request which purpose is to make sure that the new structure makes sense and that there are no regressions at places where we already use UserTable. Therefore, during the review, I'd suggest:

  • Check that there are no visual regressions in the following features:
    • Coach
      • Enroll users to a group
    • Facility
      • Facility users table
      • Coaches and learners preview tables on a class page
      • Enroll learners to a class
      • Assign coaches to a class
  • Check that there are no functional regressions when selecting multiple users, for example by enrolling learners to a group as a coach
  • Check tables raw HTML output, including its visually hidden parts in regards to accessibility, in both selectable and non-selectable mode
  • In regards to the new single selection behavior, does code for radio buttons make sense?

I will provide a way to preview the table with radio buttons in a subsequent pull request for the choose admin page if that's fine. If you want to try it out sooner, let me know, and I can provide my working branch.

Note: Since I moved the file, if you want to preview actual changes line by line, you will need to go commit by commit.

Testing checklist

  • [x] Contributor has fully tested the PR manually
  • [x] If there are any front-end changes, before/after screenshots are included
  • [ ] Critical user journeys are covered by Gherkin stories
  • [x] Critical and brittle code paths are covered by unit tests

PR process

  • [x] PR has the correct target branch and milestone
  • [x] PR has 'needs review' or 'work-in-progress' label
  • [ ] If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • [ ] If this is an important user-facing change, PR or related issue has a 'changelog' label
  • [ ] If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

MisRob avatar Aug 10 '22 13:08 MisRob

@radinamatic I'd appreciate if you could have a look at tables in regards to a11y things I mentioned in the description (cc'ed you in the relevant parts)

MisRob avatar Aug 12 '22 13:08 MisRob

@MisRob Thank you for investing effort to find a better output for the screen reader users! 🙏🏽

I will do some more tests with NVDA to ensure all the visually hidden information makes sense, but one clear issue became evident so far: while for the Select all checkbox the label is correctly configured, that is not the case for the rest of the checkboxes related to users. Could you change the span element into label with a proper for attribute? That should probably be sufficient.

Select all User
Selection_024 Selection_023

radinamatic avatar Aug 15 '22 12:08 radinamatic

Thank you @radinamatic.

while for the Select all checkbox the label is correctly configured, that is not the case for the rest of the checkboxes related to users. Could you change the span element into label with a proper for attribute? That should probably be sufficient.

I should have mentioned this louder, this is one of the KDS issues that I reported here https://github.com/learningequality/kolibri-design-system/issues/347 (linked with some other issues in the description of the PR in the references section). The reason why it works for the "Select all" checkbox is that we use the KCheckbox component in a bit different way in that case. But looking at KCheckbox again, I think I noticed something that I had missed before that I could use to deal with this better, so let me play around with it before you test it again. I will let you know.

MisRob avatar Aug 15 '22 12:08 MisRob

@radinamatic No, so it didn't work. I think it'd be best to prioritize fixing https://github.com/learningequality/kolibri-design-system/issues/347, it should be straightforward and it will also fix other places in Kolibri where we lack checkboxes labels.

MisRob avatar Aug 15 '22 13:08 MisRob

@radinamatic Taking into account that it's one of the most important a11y things I'll go ahead and set P0 on that issue and make sure to mention it during the next iteration planning

MisRob avatar Aug 15 '22 13:08 MisRob

Now I realize that the previous implementation may be in part governed by the referenced KDS limitation. However, would rather not go back even though this would be temporary regression since it had other problems, it complicated work on radio buttons, and avoiding fixing this in KDS affects negatively more places. I think it'd be fine to leave this PR open though until we address it in KDS to make sure it's okay.

MisRob avatar Aug 15 '22 14:08 MisRob

I'm all for pushing up for a solution in KDS that would have a broader reach, compared with patching things up separately in various places. Keeping an 👁️ on the PR that you mention, hit me when it needs testing! 🙏🏽

radinamatic avatar Aug 15 '22 22:08 radinamatic

Rebased on top of the latest develop that contains fixed KCheckbox so this is ready for review and testing again @radinamatic and @marcellamaki

MisRob avatar Aug 30 '22 08:08 MisRob

All the checkboxes are now properly labelled, thank you!!! 👏🏽 💯

2022-09-02_10-32-57

radinamatic avatar Sep 02 '22 08:09 radinamatic

Reverting the merge as it was merged by mistake and it also broken tests on develop

MisRob avatar Sep 02 '22 12:09 MisRob