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

Update UserTable to allow single selection #9597

Merged
merged 6 commits into from
Sep 2, 2022

Conversation

MisRob
Copy link
Member

@MisRob MisRob commented Aug 10, 2022

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):

  • Moves the table to core components since it's used by more plugins
  • Related accessibility improvements of the selectable table (cc @radinamatic)
    • 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. Based on recommendations from this WebAIM archive thread.

References

Related #9326

Opened KDS issues related to some limitations I experienced:

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

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

PR process

  • PR has the correct target branch and milestone
  • 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 MisRob added the work-in-progress Not ready for review label Aug 10, 2022
@MisRob MisRob added this to the 0.16.0 milestone Aug 10, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Aug 10, 2022

@MisRob MisRob force-pushed the user-table-single-selection branch from 6124f61 to 0220a50 Compare August 11, 2022 08:36
@MisRob MisRob marked this pull request as ready for review August 12, 2022 13:28
@MisRob MisRob added TODO: needs review Waiting for review TAG: a11y Affecting accessibility DEV: frontend and removed work-in-progress Not ready for review labels Aug 12, 2022
@MisRob
Copy link
Member Author

MisRob commented Aug 12, 2022

@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)

@radinamatic
Copy link
Member

@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

@MisRob
Copy link
Member Author

MisRob commented Aug 15, 2022

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 learningequality/kolibri-design-system#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
Copy link
Member Author

MisRob commented Aug 15, 2022

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

@MisRob
Copy link
Member Author

MisRob commented Aug 15, 2022

@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
Copy link
Member Author

MisRob commented Aug 15, 2022

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.

@radinamatic
Copy link
Member

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! 🙏🏽

@MisRob MisRob added work-in-progress Not ready for review and removed TODO: needs review Waiting for review labels Aug 16, 2022
@MisRob MisRob marked this pull request as draft August 16, 2022 12:32
MisRob added 6 commits August 30, 2022 10:35
the table markup and provide the visually hidden column
header on its previous place in markup to make sure that
the purpose of the table column containing checkboxes
to select users gets communicated to assistive technologies.
The select all checkbox retains its position visually but
it is achieved rather by means of CSS.
and improve selectable table accessibility
by using full names as radio buttons and
checkboxes labels.
@MisRob MisRob force-pushed the user-table-single-selection branch from d74e01b to c2cedc6 Compare August 30, 2022 08:42
@MisRob MisRob marked this pull request as ready for review August 30, 2022 08:44
@MisRob MisRob removed the work-in-progress Not ready for review label Aug 30, 2022
@MisRob MisRob added the TODO: needs review Waiting for review label Aug 30, 2022
@MisRob
Copy link
Member Author

MisRob commented Aug 30, 2022

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

@MisRob MisRob requested a review from jredrejo August 31, 2022 07:06
@radinamatic
Copy link
Member

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

2022-09-02_10-32-57

Copy link
Member

@radinamatic radinamatic left a comment

Choose a reason for hiding this comment

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

Major accessibility issue fixed and included in this PR, great progress! 👍🏽 :shipit:

@MisRob MisRob merged commit 371bd8b into learningequality:develop Sep 2, 2022
@MisRob
Copy link
Member Author

MisRob commented Sep 2, 2022

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

@MisRob MisRob deleted the user-table-single-selection branch February 17, 2023 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DEV: frontend TAG: a11y Affecting accessibility TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants