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

update autocomplete to be more comprehensive #166

Merged
merged 1 commit into from
Feb 3, 2024

Conversation

hanayik
Copy link
Contributor

@hanayik hanayik commented Jul 8, 2021

It does not break early, rather it now extends the result list

It does not break early, rather it now extends the result list
@hanayik hanayik requested a review from pjvandehaar July 8, 2021 10:27
@pjvandehaar
Copy link
Collaborator

pjvandehaar commented Jul 8, 2021

I initially limited the autocomplete to 10 results because I think that scrolling dropdowns confuse users. But when I search for a common string like “cancer” or “diabetes” it would be nice to see more of the results. 👍

Two comments then:

  1. I think

    will show only 100 results, so either raise that to 1000 or lower this to 100.

  2. Is it slow? On a big dataset will it take >100ms? If you haven’t checked this I can try. I’m mostly worried about rsid autocomplete. Fast autocomplete is important to me, and perhaps 1000 is too much on slow internet. We’ll have to test it.

@hanayik
Copy link
Contributor Author

hanayik commented Jul 8, 2021 via email

@jhchung
Copy link

jhchung commented Jul 26, 2021

Hi @pjvandehaar,

I had the idea to do the same change for search and saw this pull request from @hanayik.

My changes are very similar to Taylor's, but I added some options to conf.py to allow the user to changing the total number of results and whether to extend or break out of autocomplete.

Not sure how to go about making a pull request because we touched the same bit of code.

Best,
-Jonathan

Copy link
Collaborator

@pjvandehaar pjvandehaar left a comment

Choose a reason for hiding this comment

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

Looks good.

@pjvandehaar pjvandehaar merged commit fe11eaf into statgen:master Feb 3, 2024
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.

3 participants