Skip to content

Switch search functionality to custom handling #2486

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

Merged
merged 10 commits into from
Jan 28, 2021

Conversation

parlough
Copy link
Member

@parlough parlough commented Jan 23, 2021

This switches the current typeahead handling to custom handling. The typeahead library had a lot of functionality we don't use, it isn't supported anymore, and it also required us to also pull in jquery.

This is a preliminary change and sticks with the same search functionality and UI. Follow-up PRs can introduce style changes as well as expanded search functionality.

This PR accomplishes the following:

  • Removes the typeahead library
  • Removes jquery and rewrites our code relying on it
  • Writes custom search handling based off our index.json
  • Avoids querying for the index.json multiple times unnecessarily.
  • Fixes Can't type slash in the search field #2483

Note: These changes don't support Internet Explorer. Testing previous functionality, it didn't before either, so nothing is really lost here.

Breaking changes:

  • Removes jquery library
  • Removes typeahead(and bloodhound as a result) library

Testing instructions:

  1. First, clone this branch locally.
  2. Serve some documentation such as the SDK docs: pub run grinder serve-sdk-docs
  • Verify searching at least works in all of the fields
  • Verify the / works to grab focus and you are still able to type a slash in the input field
  • Verify the suggestions are visible and properly positioned
  • Verify you can move up and down between the suggestions to move between them. Also verify the text in the input field switches to each suggestion's name, but if you don't select any and click away from the search field the typed value remains.
  • Verify moving to a suggestion with your arrow keys and clicking enter opens that suggestion's page.
  • Without using arrow keys, verify you can see a hint(such as forEach when you type for) behind your written text. Proceed to verify clicking Tab fills the search bar with that hint.
  • Verify you can click a suggestion with the mouse to open the appropriate page.
  • Regression test any other functionality you expect from the search fields(s).

@google-cla google-cla bot added the cla: yes Google CLA check succeeded. label Jan 23, 2021
@coveralls
Copy link

coveralls commented Jan 23, 2021

Coverage Status

Coverage increased (+0.01%) to 91.62% when pulling c66f366 on parlough:feature/remove-jquery-typeahead into 0ba8dc8 on dart-lang:master.

@parlough parlough changed the title [WIP] Switch search functionality to custom handling Switch search functionality to custom handling Jan 23, 2021
@parlough parlough marked this pull request as ready for review January 23, 2021 21:53
@jcollins-g
Copy link
Contributor

adding @srawlins to reviewers as I am not that knowledgeable with javascript

@parlough
Copy link
Member Author

I'm not the most familiar either, so if anything isn't clear from the code let me know!

Copy link
Contributor

@jcollins-g jcollins-g left a comment

Choose a reason for hiding this comment

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

Some general questions, I'll also take the code for a spin locally.

@srawlins
Copy link
Member

Two broad questions:

  1. Is there a different state-of-the-art which has replaced typeahead? Should we not be using that?
  2. Excellent manual testing instructions. Did you investigate automated testing? There might be a few ways we could test in Chrome or Firefox on GitHub actions, with a dummy HTML page perhaps. Not necessary to land, but it would be great to think of a plan going forward. @jcollins-g thoughts?

@jcollins-g
Copy link
Contributor

Automated testing would be a great addition. We've needed it for a long while and agree that it can go in a future PR. This area of the code has been a bit fragile and relying on manual spot checking shouldn't be the only way we test this functionality.

@parlough
Copy link
Member Author

Two broad questions:

  1. Is there a different state-of-the-art which has replaced typeahead? Should we not be using that?
  2. Excellent manual testing instructions. Did you investigate automated testing? There might be a few ways we could test in Chrome or Firefox on GitHub actions, with a dummy HTML page perhaps. Not necessary to land, but it would be great to think of a plan going forward.
  1. I did take a look to see if there were any decent, modern replacements that were more on the slim side. There wasn't really anything decent from what I saw, at least that didn't rely on jquery. Plus we have a decent amount of custom scoring/handling and eventually may want to add further search flexibility, so I saw custom handling as the easiest way to go.

  2. I agree some form of automatic testing would be nice and I'll take a look to see what my options are here. Perhaps to land later as you said, but I'll definitely investigate this and other improvements :)

@srawlins
Copy link
Member

  1. I did take a look to see if there were any decent, modern replacements that were more on the slim side. There wasn't really anything decent from what I saw, at least that didn't rely on jquery. Plus we have a decent amount of custom scoring/handling and eventually may want to add further search flexibility, so I saw custom handling as the easiest way to go.

Ah yeah the custom scoring etc is probably a good reason to roll our own.

  1. I agree some form of automatic testing would be nice and I'll take a look to see what my options are here. Perhaps to land later as you said, but I'll definitely investigate this and other improvements :)

Thanks much! From previous experience, I think anything dealing with keystroke or mouse events cannot be done in a headless browser like PhantomJS. We need something bigger like Chrome. But I think with a dummy HTML file, we should be able to write some straightforward tests and run them with Karma or something.

Copy link
Contributor

@jcollins-g jcollins-g left a comment

Choose a reason for hiding this comment

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

Finished review: One change. Did some spot checks and verified #2483 is fixed.

It looks like tabbing while within the search box is no longer equivalent to clicking on a selection, which is a good thing. I also notice that the search does substrings now which is a welcome change, and just in general it "feels" better to use than the old one, in subtle ways. Looking forward to this change landing!

@parlough parlough force-pushed the feature/remove-jquery-typeahead branch from 8567142 to c66f366 Compare January 28, 2021 16:02
@jcollins-g
Copy link
Contributor

Force pushing makes me have to re-review everything to avoid missing changes that could have been introduced, just an FYI that will slow the landing of this commit.

@parlough
Copy link
Member Author

@jcollins-g Sorry about that, I'll keep that in mind for next time!

@srawlins
Copy link
Member

What is the alternative to force pushing after a rebase? Should I not be rebasing?

@parlough
Copy link
Member Author

@srawlins You can merge the updated master into your branch. If not as clean of a history in the PR, but will all be squashed on merge anyway.

@jcollins-g
Copy link
Contributor

@srawlins You can merge the updated master into your branch. If not as clean of a history in the PR, but will all be squashed on merge anyway.

Yes. While this is not considered "best practice" for git itself in many situations because of the history being complicated, it actually works better with the review tools on github to do it this way and depend on the later "squash and merge" to clean up the mess.

@srawlins
Copy link
Member

OK I'll merge from now on.

@jcollins-g jcollins-g merged commit f51278b into dart-lang:master Jan 28, 2021
@srawlins
Copy link
Member

This may have fixed #1776 ; I'll check later on pub.dev.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Google CLA check succeeded.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't type slash in the search field
4 participants