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

Search box autocomplete does not give a full list of results #1367

Closed
MikkoAleksanteri opened this issue Sep 27, 2022 · 8 comments · Fixed by #1369
Closed

Search box autocomplete does not give a full list of results #1367

MikkoAleksanteri opened this issue Sep 27, 2022 · 8 comments · Fixed by #1369
Labels
Milestone

Comments

@MikkoAleksanteri
Copy link
Contributor

At which URL did you encounter the problem?

https://finto.fi/yso/fi/

What steps will reproduce the problem?

  1. Go to the frontpage of YSO vocabulary at finto.fi
  2. Use the search box to type "sosiaalinen", wait for the autocomplete list to open

What is the expected output? What do you see instead?

Expected to see the full list of results (according to our vocabulary experts), instead I see only 29 first results

image

Maybe something we broke during Bootstrap update?

What browser did you use? (eg. Firefox, Chrome, Safari, Internet explorer)

Firefox (102.3.0esr (64-bit))

@MikkoAleksanteri MikkoAleksanteri added this to the Next Tasks milestone Sep 27, 2022
@kinow
Copy link
Collaborator

kinow commented Sep 27, 2022

The network call is https://finto.fi/rest/v1/search?query=sosiaalinen*&vocab=yso&lang=fi&labellang=fi, and it brings 72 results.

@kinow
Copy link
Collaborator

kinow commented Sep 27, 2022

There doesn't seem to be anything “hidden” (or not displayed) in the DOM, so my best guess for now is that the PHP templates may be missing something (possibly this bug was introduced during the Bootstrap issue 👍 ).

@kinow
Copy link
Collaborator

kinow commented Sep 28, 2022

Interesting. I modified the code adding a :render callback, that prints the elements returned for rendering.

    }).on('typeahead:render', function($e) {
      var args = [].slice.call(arguments, 1)
      console.log($e)
      console.log(args)
    })

There are indeed 29 elements. The limit in the dataset configuration is set to 100 (the variable autocomplete_limit).

I replaced the variable by 100, got the same 29 elements. Replaced now by 101 and got 30 elements 🤔

@kinow
Copy link
Collaborator

kinow commented Sep 28, 2022

Huh… possibly this opened pull request: twitter/typeahead.js#1774

A possible workaround would be to modify the config.js file, setting the autocomplete_limit to a higher value? I tried monkey patching that function, but it uses a canceled variable that exists within the context of the function, but not in an object that I can access and refer in a monkey-patched code (at least not without making a huge macgyverism that I wouldn't trust even if that worked 🤣 )

@osma
Copy link
Member

osma commented Sep 28, 2022

Whoa, thanks for the awesome detective work @kinow !

If that typeahead problem is indeed the cause, then we're in a difficult situation, as typeahead isn't being maintained and even the PR with the proposed fix is from 2019. We can't really expect that PR to ever be merged.

The comments in that PR mention adjust as some kind of workaround, is that the same workaround you are thinking of?

@osma
Copy link
Member

osma commented Sep 28, 2022

One possible fix is also to just copy the parts of typeahead.js we need into the Skosmos codebase and fix it there, using that PR branch instead of the original broken code from the 0.11.1 release. It's obvious we need to switch away from typeahead.js (see #1349) but that's a big undertaking, not something you do just to fix a single bug.

@kinow
Copy link
Collaborator

kinow commented Sep 28, 2022

Whoa, thanks for the awesome detective work @kinow !

If that typeahead problem is indeed the cause, then we're in a difficult situation, as typeahead isn't being maintained and even the PR with the proposed fix is from 2019. We can't really expect that PR to ever be merged.

The comments in that PR mention adjust as some kind of workaround, is that the same workaround you are thinking of?

I don't know why they marked the adjust in the reply. There is no adjust in the documentation (nor in the code https://github.com/twitter/typeahead.js/search?q=adjust).

I had found another bug in typeahead.js during the migration to Bootstrap. I think that took probably ~20% of the total time of the migration work 😠 The solution was this one 😬 :

image

One possible fix is also to just copy the parts of typeahead.js we need into the Skosmos codebase and fix it there, using that PR branch instead of the original broken code from the 0.11.1 release. It's obvious we need to switch away from typeahead.js (see #1349) but that's a big undertaking, not something you do just to fix a single bug.

That could work, I think. The issue is that the code is being processed with grunt. So we could either build it with our changes, producing a minified and production ready version, or use the development build modifying the lines we need. Downside is that without minifying it could slow down a little the loading time.

@mirmaid
Copy link
Collaborator

mirmaid commented Sep 28, 2022

For digi, autocomplete gives only 5 results (if you hit search you get 48). When you continue writing digit, etc. you get more hits in the autocomplete list, instead of first getting the most hits and then less and less when you continue to type more letters...

@Vainonen Vainonen modified the milestones: Next Tasks, 2.16 Oct 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants