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

Web Apps: switched combobox to select2 #28701

Merged
merged 19 commits into from Nov 16, 2020
Merged

Conversation

orangejenny
Copy link
Contributor

@orangejenny orangejenny commented Oct 27, 2020

SUMMARY

https://dimagi-dev.atlassian.net/browse/USH-394

The atwho widget used for comboboxes is not at all accessible to screen readers. atwho is no longer maintained, so that's not going to improve.

select2's upcoming release has a number of accessibility improvements, although it isn't yet released and there's some debate about whether those improvements will be sufficient. My hope is that switching over to select2 will be enough of an improvement for the functionality needed in web apps, which is pretty basic. At a minimum, select2 is at least backed by a select element instead of a text box. We'll be testing this from an accessibility perspective and can consider additional changes, possibly switching to selectWoo, if necessary.

This PR also updates "minimal" select questions, which were previously using plain HTML select elements, to use select2 for the sake of consistency. This makes the minimal and combobox options on web apps very similar, with the only difference being that the combobox allows for selecting the specific matching algorithm, whereas minimal uses select2's default search.

RISK ASSESSMENT / QA PLAN

QA ticket

PRODUCT DESCRIPTION

Makes fairly minor changes to the appearance both both comboboxes and "minimal" select questions in web apps.

Before, comboboxes use atwho and minimal questions use standard HTML dropdowns:
Screen Shot 2020-10-28 at 12 36 52 PM

Screen Shot 2020-10-28 at 12 37 21 PM

After, both widgets use select2:
Screen Shot 2020-10-28 at 12 39 10 PM

Screen Shot 2020-10-28 at 12 24 31 PM

@orangejenny orangejenny added awaiting QA QA in progress. Do not merge product/all-users-all-environments Change impacts all users on all environments labels Oct 27, 2020
@orangejenny
Copy link
Contributor Author

FYI @dimagi/product

No longer necessary because select2 doesn't allow you to enter free text.
return "";
};

self.options = ko.computed(function () {
Copy link
Member

Choose a reason for hiding this comment

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

i'm guessing pureComputed doesn't have the subscribe option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does, but the subscribers shouldn't have side effects. I think that rendering the select2 counts as a side effect, although I'm totally certain. I'm looking at the "When not to use a pure computed observable" section in these docs: https://knockoutjs.com/documentation/computed-pure.html

Copy link
Member

@biyeun biyeun left a comment

Choose a reason for hiding this comment

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

this is looking great. is accessibility part of a USH initiative / is there some plan to looking into moving away from atwho on vellum? I'm sad another useful js library has now fallen into the unmaintained category. 😞

@orangejenny
Copy link
Contributor Author

@biyeun Agreed, I like atwho. Although some of the places we use it, like this one, the underlying concept is a dropdown and we only used atwho because the UI is lighter-weight than the old version of select2. In these places, I'm happy to replace with new select2. I think that applies to vellum's usages of atwho. Places where atwho is really being used as it's intended, like in the web apps debugger where it autocompletes function names in xpath expressions, are trickier.

As far as planning/resourcing, USH is indeed working on accessibility, but we're focused on web apps usage, so we're unlikely to update vellum anytime soon. Let's offline if you want to chat more about this.

@dimagimon dimagimon added the Risk: Medium Change affects files that have been flagged as medium risk. label Nov 3, 2020
@orangejenny orangejenny added QA Passed awaiting QA QA in progress. Do not merge and removed awaiting QA QA in progress. Do not merge QA Passed labels Nov 4, 2020
} else {
return null;
}
}

Choose a reason for hiding this comment

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

Missing trailing comma. (comma-dangle)

Even though the UI no longer allows invalid answers since it doesn't support free text,
it's still possible for another question to update a combobox with an invalid answer.
This validation is now available to both comboboxes and dropdowns.
Copy link
Member

@biyeun biyeun left a comment

Choose a reason for hiding this comment

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

🥳

Without this, web apps initially sets the value of an empty combobox to null,
but then the knockoout value binding sets it to undefined, which counts as a
change and triggers answer validation.
This undoes 36ac455 and 0c19009, again removing the validation code
because dropdowns don't allow invalid selections. What those commits
missed is that the combobox needed to be updated to use the index for
rawAnswer. Previously, the combobox was backed by a plain text input,
which meant rawAnswer was text and the entry would look up that text's
index. Now that there's a dropdown, both the validation and the lookups
are no longer necessary. The only logic dealing with text is receiveMessage.
Previously, if options changed, the minimal dropdown would select whatever
option was at the same index as the previous answer, and the combobox
would should an 'invalid choice' error.
@orangejenny
Copy link
Contributor Author

@biyeun Requesting review once more, please - the last few commits changed validation logic a bit.

@orangejenny orangejenny added QA Passed and removed awaiting QA QA in progress. Do not merge labels Nov 16, 2020
@orangejenny orangejenny merged commit 923025e into master Nov 16, 2020
@orangejenny orangejenny deleted the jls/combobox-select2 branch November 16, 2020 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/all-users-all-environments Change impacts all users on all environments QA Passed Risk: Medium Change affects files that have been flagged as medium risk.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants