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

Replace Chosen with accessible-autocomplete #1161

Merged
merged 9 commits into from
Feb 1, 2021
Merged

Replace Chosen with accessible-autocomplete #1161

merged 9 commits into from
Feb 1, 2021

Conversation

hbillings
Copy link
Member

@hbillings hbillings commented Aug 31, 2020

Description

Closes #1109. Chosen.js is not accessible; replace it with accessible-autocomplete.
Todo:

  • Tests.
  • Figure out dropdown styling.

Looks like this closed:
Screen Shot 2020-08-31 at 2 22 36 PM

And this expanded:
Screen Shot 2020-08-31 at 2 22 21 PM

I'm wondering what we'd like to do about the dropdown. Currently, Tock's dropdown separates projects into categories, although I have to admit I'm a little unsure how it's doing that:
Screen Shot 2020-08-31 at 2 24 49 PM

I'm also not clear on whether this is a feature that people would miss if it disappeared. It seems like it makes things more usable to me and like it's probably worth attempting to figure out with the new autocomplete, but I wanted to open this and get some opinions flowing.

@neilmb
Copy link
Member

neilmb commented Aug 31, 2020

I'd happily take the chosen replacement as-is and backlog a ticket to figure out categorization.

@adunkman
Copy link
Member

👋 Resident Chosenista here. Chosen does that through <optgroup> — not sure if that’s helpful, but figured I’d chime in! 😄

@hbillings
Copy link
Member Author

Okay, so it looks like the optgroups get created by Django's form element on the back end, but accessible-autocomplete doesn't pick them up. I'm inclined to say let's ship it as-is and see if people complain about it. @Jkrzy @arouault what do you think about that?

@arouault
Copy link

arouault commented Sep 2, 2020

@hbillings Hmm - having no real information about why the groupings were created in the first place, it's hard to say. Based on some research of late... This tells me categories exist because business units are important, ie, there was a situation where non-18F units were billing to 18F tock lines because they didn't know the distinction. I'd argue for keeping categories. Can we transfer over the drop down categorization in some fashion? maybe @Jkrzy or @tbaxter-18f can speak to the legacy of that feature.

@Jkrzy
Copy link
Contributor

Jkrzy commented Sep 2, 2020

The groupings are derived from 'AccountingCode's and originated in PR #23, ~5 years ago.

It also looks like accessible autocomplete doesn't have an existing path to categories. alphagov/accessible-autocomplete#145

+1 to Neil's thought, we know we need to move away from Chosen. Let's sort out if/how we need to enhance that UI after it's in place.

@hbillings
Copy link
Member Author

@Jkrzy I like the option in that example of using the custom template option to add a hint to the text. That would probably be a pretty easy enhancement (whether it needs to go in this PR or not is another story). (I share your concern about business units tocking to the wrong lines, @arouault, and I'd like to try to mitigate that if possible! I think it might require a little creativity and trial-and-error, though.)

@tbaxter-18f
Copy link
Contributor

Tocking to the wrong line should be difficult since we added the enhancement this year to only show tock lines for your org.

I know you can create Django choices with optgroups, but I don't know if the accessible autocomplete would work well with them or not.

I agree with the above. It's a feature of only limited benefit, and we can log a follow-up issue to bring it back if we want to.

@Jkrzy
Copy link
Contributor

Jkrzy commented Sep 3, 2020

@hbillings, let me know if there's a more convenient way the backend can be providing things for the new autocomplete!

@hbillings hbillings changed the title [WIP] Replace Chosen with accessible-autocomplete Replace Chosen with accessible-autocomplete Sep 9, 2020
@hbillings hbillings marked this pull request as ready for review September 9, 2020 22:48
@hbillings
Copy link
Member Author

I went back to write the test I thought this needed, and I don't actually think it needs one. We'd basically just be testing the Chosen library.

Copy link
Member

@adunkman adunkman left a comment

Choose a reason for hiding this comment

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

A few clean-up comments, but this looks good! Giving a preemptive 👍 assuming the comments are addressed.

tock/tock/static/js/components/timecard.js Outdated Show resolved Hide resolved
tock/tock/static/js/components/timecard.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@Jkrzy Jkrzy left a comment

Choose a reason for hiding this comment

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

Thanks @Sgtpluck!

@Jkrzy Jkrzy merged commit 80ac913 into main Feb 1, 2021
@Jkrzy Jkrzy deleted the hjb/remove-chosen branch February 1, 2021 15:18
Jkrzy added a commit that referenced this pull request Feb 9, 2021
This reverts commit 80ac913, reversing
changes made to 0b037bf.
tbaxter-18f pushed a commit that referenced this pull request Feb 9, 2021
* Revert "Merge pull request #1161 from 18F/hjb/remove-chosen"

This reverts commit 80ac913, reversing
changes made to 0b037bf.

* Update javascript dependencies
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Chosen.js and usability/accessibility
7 participants