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

Add autocomplete filter #65

Merged
merged 9 commits into from Sep 3, 2019
Merged

Add autocomplete filter #65

merged 9 commits into from Sep 3, 2019

Conversation

@nholden
Copy link
Member

@nholden nholden commented Sep 1, 2019

This PR adds a new autocomplete filter to the "explore the data" page that we can use for agencies and counties.

autocomplete

Things I punted on

  • Ignoring a filter group when all the individual options are unchecked. This is a behavior that exists on the current "explore the data" page, and I think we should consider implementing it in the redesign, but I thought that was best saved for a future PR.
  • Setting up a polyfill for the <datalist> element. At one point, <datalist> was not well supported across browsers, but it looks like it's actually pretty well supported now. If we have specific browser versions we're targeting, we can revisit this.
  • Disabling native autofill on Safari. This seems to be a pain for lots of folks, and I'd be down to revisit this in a separate PR if we think this is a serious usability issue.
  • Adding fake county data. I know that there's some ongoing work with our data formatting, so I didn't want to spend too much time creating fake data under our current assumptions.

supports #3

@nholden nholden self-assigned this Sep 1, 2019
@nholden nholden added this to In progress in Redesign Launch via automation Sep 1, 2019
@vercel
Copy link

@vercel vercel bot commented Sep 1, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://tji-nextjs-git-autocomplete.evaruth.now.sh

Loading

@vercel vercel bot temporarily deployed to staging Sep 2, 2019 Inactive
@vercel vercel bot temporarily deployed to staging Sep 2, 2019 Inactive
@vercel vercel bot temporarily deployed to staging Sep 2, 2019 Inactive
@vercel vercel bot temporarily deployed to staging Sep 2, 2019 Inactive
@vercel vercel bot temporarily deployed to staging Sep 2, 2019 Inactive
@vercel vercel bot temporarily deployed to staging Sep 2, 2019 Inactive
@nholden nholden changed the title [WIP] Add agency and county filters Add autocomplete filter Sep 2, 2019
@nholden nholden marked this pull request as ready for review Sep 2, 2019
@nholden nholden requested review from judan, LayaTaal and haywood Sep 2, 2019
Copy link
Contributor

@LayaTaal LayaTaal left a comment

I think this looks great @nholden! I really like how you handled this and the abstraction of the FilterContainer component.

Regarding your punts:

  • Ignoring a filter group when all the individual options are unchecked.
    • Agreed, going to make an issue for this.
  • Setting up a polyfill for the element. At one point, was not well supported across browsers, but it looks like it's actually pretty well supported now. If we have specific browser versions we're targeting, we can revisit this.
    • I went over our analytics right now and it looks like we have a fair number of users still on iOS 12.1.x versions (I didn't see any users in the last 6 months on 12.3, which seems strange to me). Aside from this browser, we are ok, but this may be something we want to consider indeed.
  • Disabling native autofill on Safari. This seems to be a pain for lots of folks, and I'd be down to revisit this in a separate PR if we think this is a serious usability issue.
    • Can you point me to an example of this? I'm not noticing anything on my desktop Safari. I also don't see very many users visiting from Safari on desktop, so it may not be worth addressing.
  • Adding fake county data. I know that there's some ongoing work with our data formatting, so I didn't want to spend too much time creating fake data under our current assumptions.
    • I think we are ok with what you've done. It looks like we should have the real data in the next few days.

Loading

@nholden
Copy link
Member Author

@nholden nholden commented Sep 3, 2019

Thanks for the review, @LayaTaal! 🙏

I went over our analytics right now and it looks like we have a fair number of users still on iOS 12.1.x versions (I didn't see any users in the last 6 months on 12.3, which seems strange to me). Aside from this browser, we are ok, but this may be something we want to consider indeed.

Oh, that's good to know! I wonder if we should pick the browsers that we're aiming to support and include them in the README. It might make it easier to decide when to use polyfills or explore other approaches.

This reminded me that I noticed an issue with the filters on mobile, so I opened #66.

Can you point me to an example of this?

Sure! Here's what I see on Safari desktop.

autocomplete-bug

Loading

@nholden nholden merged commit 369fcb1 into master Sep 3, 2019
1 check passed
Loading
Redesign Launch automation moved this from In progress to Done Sep 3, 2019
@nholden nholden deleted the autocomplete branch Sep 3, 2019
@nholden nholden mentioned this pull request Sep 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants