Skip to content

Fix typeahead and handlebars template.#1303

Merged
osma merged 1 commit intoNatLibFi:masterfrom
kinow:fix-search-autocomplete
Apr 26, 2022
Merged

Fix typeahead and handlebars template.#1303
osma merged 1 commit intoNatLibFi:masterfrom
kinow:fix-search-autocomplete

Conversation

@kinow
Copy link
Collaborator

@kinow kinow commented Apr 25, 2022

Reasons for creating this PR

Fix styles and UI behavior, broken in #1182

Link to relevant issue(s), if any

Description of the changes in this PR

I noticed that the data was present for Typeahead to display it, but for some reason we only saw part of the data when values were replaced (e.g. Catalans & Catalan People). I couldn't find anything in the Typeahead docs, but then it occurred to me that it could be now handling templates the same way Vue.js does, by forcing to have a single root-node.

I moved things under a <div> in the template rendered for each autocomplete entry, and then it finally display all the information received and used in the template.

Known problems or uncertainties in this PR

  1. I left a question in Autocomplete problems after Bootstrap 5 upgrade #1301 asking about the color of the text. This is a 1 minute change, just pending a reply over there 👍

  2. @osma I couldn't understand why my types are not being replaced by shorter text. I wonder if it's because I have been using old vocabularies I exported from finto and/or finto.dev?

I set a few breakpoints, and after clearing my local storage, I can confirm the UI is sending a request to http://localhost:9090/rest/v1/types?lang=en to retrieve types, and is also using the @context returned from the autocomplete request.

But the right-most information is still using the URI instead of a value like "General Concept" as in your screenshot attached to #1301. See my screenshot below for an example:

Screenshot from 2022-04-25 17-18-55

It shows skos:Concept, and other URI's instead of a simpler text 😞 . Thanks!

Checklist

  • phpUnit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works (if not, explain why below)
  • The PR doesn't introduce unintended code changes (e.g. empty lines or useless reindentation)

@codecov
Copy link

codecov bot commented Apr 25, 2022 •
edited
Loading

Codecov Report

Merging #1303 (2c76ebc) into master (df06b59) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #1303   +/-   ##
=========================================
  Coverage     69.44%   69.44%           
  Complexity     1657     1657           
=========================================
  Files            32       32           
  Lines          4068     4068           
=========================================
  Hits           2825     2825           
  Misses         1243     1243

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df06b59...2c76ebc. Read the comment docs.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 25, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
2.5% 2.5% Duplication

@osma
Copy link
Member

osma commented Apr 26, 2022 •
edited
Loading

Thanks @kinow , I will take a closer look!

No worries about the types showing up as URIs, that's a recurring problem. I fixed one instance of this recently (see #1254 ) but this is a common occurrence. Apparently the mechanism we have for looking up concept type labels is a bit fragile.

@osma
Copy link
Member

osma commented Apr 26, 2022

Using the code from this PR, the type labels are working for me:

kuva

I think you have an unrelated problem with those.

I tested this and everything seems to be working, so I will merge this PR. It seems to me there are still some minor font style issues (color was already mentioned, but font sizes are also a bit different from before) but those can be handled in separate PRs.

@osma osma merged commit dd94d44 into NatLibFi:master Apr 26, 2022
@osma osma mentioned this pull request Apr 26, 2022
Closed
@kinow kinow deleted the fix-search-autocomplete branch April 26, 2022 08:37
@osma osma self-assigned this Apr 27, 2022
@osma osma added the bug label Apr 27, 2022
@osma osma added this to the 2.15 milestone Apr 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@osma osma

Labels

bug

Projects

None yet

Milestone

2.15

Development

Successfully merging this pull request may close these issues.

Autocomplete problems after Bootstrap 5 upgrade

2 participants

@kinow @osma