Skip to content

Conversation

@AlexVelezLl
Copy link
Member

Summary

image

References

Closes #13408.

Reviewer guidance

I have created a dummy side panel in the facilities plugin > classes tab. So go to facilities > classes and check the list behaviour with dummy data.

@github-actions github-actions bot added APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) DEV: frontend SIZE: medium labels Jun 5, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Jun 5, 2025

@MisRob
Copy link
Member

MisRob commented Jun 9, 2025

Would you please test @radinamatic?

@MisRob MisRob added the TODO: needs review Waiting for review label Jun 9, 2025
Copy link
Member

@MisRob MisRob left a comment

Choose a reason for hiding this comment

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

Code reads very easily, and uses all the approaches we agreed to use. I haven't noticed anything suspicious. Very nice work here @AlexVelezLl.

I think I will leave @nucleogenesis or @LianaHarris360 to be a main code reviewer.

As for a11y, we will see what @radinamatic's testing reveals. I haven't looked into details, however the overall approach is well aligned with what we all chatted about 😊


function handleFocusNavigation(key) {
const diff = key === 'ArrowDown' ? 1 : -1;
// adding options.length and using modulo to wrap around
Copy link
Member

Choose a reason for hiding this comment

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

🙏

const { resultsCount$ } = searchAndFilterStrings;

watch(filteredOptions, newOptions => {
sendPoliteMessage(resultsCount$({ count: newOptions.length }));
Copy link
Member

Choose a reason for hiding this comment

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

👍

type: Array,
required: true,
validator: options =>
validateObject(
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

Code changes LGTM - let's wait for @radinamatic & @pcenov to QA before approval, but everything codewise looks great!

@pcenov pcenov self-requested a review June 10, 2025 08:47
Copy link
Member

@pcenov pcenov left a comment

Choose a reason for hiding this comment

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

Thanks @AlexVelezLl - looks great, we'll further test it once it's fully integrated.

@radinamatic
Copy link
Member

Great work @AlexVelezLl ! 👏🏽

If we could, there are these two important things to achieve that will, I believe, improve even the example on the APG site 😅 (that you implemented exactly):

a) when user navigates to an option NVDA rightfully announces that it is an item in the list, and its position:

Select an option  list
Option 1  1 of 15

However, even though the list label says Select an option, user is not explicitly informed that the option in focus is actionable/clickable, and I believe we should make that clearer. One possibility would be to make it that NVDA announces clickable after each of the items, same as it would after a checkbox, but that might be a bit verbose and repetitive. Would it be possible to add the Options are clickable string to be announced by NVDA:

Select an option  list  Options are clickable
Option 1  1 of 15

b) more surprisingly for me was to find (it is extant in the APG example too) that when the option is deselected, that change of state is not announced by NVDA 😕

deselect-not-announced

I would expect that deselecting an option, when the aria-selected property changes to false would also be announced, same as when it changes to true. So I'm again 😕

Other possible points of improvement;

  1. Complete or improve the screen reader output when selecting all options. Checkbox itself works without issues, and the change of state is properly announced, but it would be beneficial to the user to be informed of the full outcome of them checking the checkbox 😉

    N-selected

My suggestion would be to pass to the screen reader the All N options are selected when the user checks the Select all options checkbox, and No options selected when they uncheck the checkbox.

  1. When the list of options is long, and some are below the fold, the page does not scroll when keyboard user is navigating to those options below the fold, and they cannot see where they are. Screen reader users might not be inconvenienced, but sighted keyboard users are 🙂

@AlexVelezLl
Copy link
Member Author

Hi @radinamatic! Just pushed some changes that should fix the points you mentioned :). Would be great if you can take a look at it again 👐.

@radinamatic
Copy link
Member

I feel like Aladdin: all I have to do is express a wish and the genie @AlexVelezLl will grant it! 🤩 👏🏽 🙇🏽‍♀️
(and I definitively had maaaaany more than 3 already 😅)

all-done

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.

So great to see this, excellent job! 💯 🎉 :shipit:

@nucleogenesis nucleogenesis merged commit 1fb1c41 into learningequality:develop Jun 11, 2025
51 checks passed
@AlexVelezLl AlexVelezLl deleted the selectable-list branch June 11, 2025 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) DEV: frontend SIZE: medium TODO: needs review Waiting for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bulk User SidePanel: Filterable & selectable list component

5 participants