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
Conversation
FYI @dimagi/product |
No longer necessary because select2 doesn't allow you to enter free text.
return ""; | ||
}; | ||
|
||
self.options = ko.computed(function () { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this 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. 😞
@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. |
} else { | ||
return null; | ||
} | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
@biyeun Requesting review once more, please - the last few commits changed validation logic a bit. |
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:
After, both widgets use select2: