Vocabulary search list Twig-templates #1470

joelit · 2023-05-31T17:21:16Z

Reasons for creating this PR

Twig template for vocabulary search

Link to relevant issue(s), if any

Fixes #1483

Description of the changes in this PR

This PR is built on the PR #1469 as it uses the same search-results.inc

Layout for the vocabulary search page
Uses the same search-results.inc as the global search (PR Global Search Results twig-templates #1481)

Known problems or uncertainties in this PR

The header is lacking the search bar
The search result filter sidebar is still just a placeholder
Copy-to-clipboard component needs to be written
Colour scheme configuration component needs to be written to global configuration

Checklist

phpUnit tests pass locally with my changes
I have added tests that show that the new code works, or tests are not relevant for this PR (e.g. only HTML/CSS changes)
The PR doesn't reduce accessibility of the front-end code (e.g. tab focus, scaling to different resolutions, use of .sr-only class, color contrast)
The PR doesn't introduce unintended code changes (e.g. empty lines or useless reindentation)

sonarqubecloud · 2023-05-31T17:22:17Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
14 Code Smells

No Coverage information
0.0% Duplication

codecov · 2023-05-31T17:28:25Z

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (e00c2b6) 70.51% compared to head (a0ccea6) 70.51%.

Additional details and impacted files

@@             Coverage Diff              @@
##             skosmos-3    #1470   +/-   ##
============================================
  Coverage        70.51%   70.51%           
  Complexity        1642     1642           
============================================
  Files               32       32           
  Lines             4314     4314           
============================================
  Hits              3042     3042           
  Misses            1272     1272

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

joelit · 2023-08-24T17:07:58Z

Search-results.inc produces more or less similar html compared to the static examples

The whitespace control of foreign labes could be improved
Gettext translations are needed to display all of the properties for a given search result

osma

Looks like a good start, I gave some quick comments.

Please make sure the PR description is up to date.

cypress/e2e/vocab-search.cy.js

src/view/global-search.twig

src/view/search-results-filter.inc

src/view/search-results.inc

src/view/vocab-search.twig

src/view/base-template.twig

osma · 2023-08-31T18:51:22Z

There's something wrong with this PR, there are now over 100 commits. I think it should be rebased on current skosmos-3 branch.

osma · 2023-09-19T12:38:25Z

I tested this locally. Some quick comments:

The color-scheme-*.css files are unnecessary in this PR and should be removed. (We will need them later, but let's keep that out of scope for this PR)
You should make sure that the Cypress test works now that PR Use dockerized Skosmos for Cypress tests #1509 was merged. Probably you will at least need to switch the test to use another test vocabulary since we don't currently have a yso vocabulary for the phpunit/cypress tests. (Alternatively, you could wait until we have a YSO subset vocabulary in place for the tests.)

Layout issues

I found some layout issues in the templates:

When searching YSA for kissa, the first result looks like this:

Problems:

The header font looks smaller than the rest of the text. I didn't check what it should be but I would expect it to be at least as large.
This concept has no labels in other languages, yet the globe icon is displayed.
The mappings to Allärs and YSO are displayed, but shouldn't there be an icon next to them?

I did the same search for KOKO and got this:

Problems:

The types are not separated by commas.

I also searched YSO for flunssa and got this:

Problems:

I would expect that the matching label "flunssa" would be displayed in the same font as "nuhakuume" (EB Garamond).

joelit · 2023-09-21T17:16:12Z

Good points @osma ! I should have tested the template with vocabularies other than YSO. I resolved all of the above, and made some minor tweaks.

Global search implemented Updated property icons Initial commit Layout for the vocabulary search page hard-coded colour scheme css CSS tweaks Overhaul of search-results.inc Fixed a typo Removed debug info CSS tweaks Fixed a HTML tag Remove colour definitions from the Twig layer Added cypress tests for vocab search Added a missing newline to the end of the file Added Skosmos variant color sheme CSS files CSS font size tweak Vocab search translations Global search twig template Global search implemented Updated property icons Initial commit Layout for the vocabulary search page CSS tweaks Overhaul of search-results.inc Removed debug info CSS tweaks Fixed a HTML tag Remove colour definitions from the Twig layer Translation templates Working cypress test for the search result list CSS tweaks Fixed bugs in the twig template logic

osma

Getting better. A few brief comments:

There seems to be something wrong with the commit history. I see duplicated commits and I cannot merge this from the command line (although GitHub doesn't indicate a merge conflict). I suggest doing a rebase+squash, combining all current work into a single commit against the current skosmos-3 branch.
The title of the page doesn't reflect that a search has been performed. E.g. on Skosmos 2, the page title is constructed as Finto: YSO 'kissa' (service name, vocabulary and search term). A similar title should be used.
I gave you slightly wrong advice on the header font for matched terms (e.g. flunssa) in the previous review. The font of the matching label should be the one used for regular text, same size and styling.
The arrows for broader/narrower do not match the layout spec. They should be rotated. Also, the alignment of icons is currently problematic, I think that center-align could be better.
Long lines are wrapped in an ugly way. They should be truncated, with a possibility to click them to reveal the full line. (This could be turned into a separate issue.)
For terms in other languages, the prefLabel should be emphasized (bold style, different color).
The HTML contains many span elements and uses of versal class. These are remnants from Skosmos 2 which are unnecessary. IMHO we should try to use simpler HTML structure, as in the static example.
The copy to clipboard button should be white
The URI text/icon is too large

…emphasis fixed

joelit · 2023-09-27T09:39:42Z

osma

Again getting much better. But I do have more comments!

PR title

Could you change the PR title to something a bit more informative?

global-search.inc

Is this file actually needed in this PR? It seems like a preliminary version of the global search template (e.g. doesn't include translations).

General page layout

There's a lot of empty space above the search result content:

Compare this with the Finto spec:

Extra language tags & untranslated types

I searched YSO for "vaatteet" and got this:

For some reason, there are (fi) language tags (without a preceding space) on the third narrower concept and all later ones, but not the first and second. (This happens with Skosmos 2 as well). I don't think they should be there or at least it should be consistent.

Also, the types isothes:ThesaurusArray and skos:Collection are not properly translated. (This is a regression compared to Skosmos 2.)

Extra whitespace and padding

The result for "kisat" looks like this:

I've selected the altlabels to show that there is an extra space before "kilvat". The same happens with all other property values as well. It would be better to omit this space (removing unnecessary whitespace from the template or using twig whitespace control).

But I think there should be slightly more padding between the icon and the text (best added after removing the whitespace). Compare with the Finto spec:

Possibly unnecessary functionality in the template

I've pointed out in the comments a couple of if clauses which I think are unlikely to be ever true (deprecated concepts & submembers). Unless there is a known use for these, I think these should be left out. (And if there is a use, a Cypress test to verify the functionality would be great!)

Cypress tests

The current test is very minimal. I suggested in a comment some additional test cases that could be implemented. I realize that making the tests may require the "mini YSO" test vocabulary that is currently being developed (see #1516).

src/view/global-search.twig

src/view/search-results.inc

osma · 2023-09-27T12:54:52Z

tests/cypress/template/vocab-search.cy.js

This is a pretty minimal test for what is a fairly advanced page type. Could we come up with more tests?

Examples:

the "X results for Y" text is there, with the correct values for X and Y

the search returns the correct number of search-result DIV elements

for one of the search results, verify the basic information, e.g.

heading information (also check that the link points where it should)

prefLabels in all languages

altLabels

broader / narrower / related

group

URI

same as above for non-Concept types e.g. ThesaurusArray ("vaatteet vuodenajan mukaan")

I added more texts for the search result properties. We should maye wait for test-yso before writing tests for the non-concept types.

sonarqubecloud · 2023-09-28T11:02:05Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
1 Security Hotspot
17 Code Smells

No Coverage information
0.0% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

osma

LGTM

joelit added this to the 3.0 milestone Aug 24, 2023

joelit self-assigned this Aug 29, 2023

joelit marked this pull request as ready for review August 31, 2023 07:22

joelit mentioned this pull request Aug 31, 2023

Issue1436 global search template #1469

Open

4 tasks

osma requested changes Aug 31, 2023

View reviewed changes

joelit force-pushed the issue1436-vocab-search-template branch from 7c1afa5 to f265f1e Compare September 1, 2023 08:16

joelit mentioned this pull request Sep 18, 2023

Vocab search field in header bar #1514

Open

60 tasks

joelit force-pushed the issue1436-vocab-search-template branch from a50f656 to cbae060 Compare September 21, 2023 17:12

joelit force-pushed the issue1436-vocab-search-template branch from cbae060 to 003179a Compare September 26, 2023 08:16

osma requested changes Sep 26, 2023

View reviewed changes

joelit and others added 5 commits September 26, 2023 11:24

Remove the superfluous versal class

b97e240

CSS tweaks to font awesome icons

de764e0

Search result font changes

71360f9

Fixed page title

77f8a4f

Styling removed, intendations somewhat fixed, preflabel and altlabel …

755aab9

…emphasis fixed

osma requested changes Sep 27, 2023

View reviewed changes

joelit added 2 commits September 27, 2023 22:04

Layout tweaks

db5a24d

Removed global search template

7b489ae

joelit added 6 commits September 27, 2023 22:29

Removed legacy logic

e57a922

Renamed CSS class

fe2f1ff

Tweaked top margin size for search results

2b077ad

Added missing translation filter

307620b

More cypress tests

ccda5ac

Layout tweaks

ca786fb

joelit changed the title ~~Issue1436 vocab search template~~ Vocabulary search list Twig-templates Sep 28, 2023

Removed unused logic

a0ccea6

osma approved these changes Sep 28, 2023

View reviewed changes

joelit merged commit 408ae61 into skosmos-3 Sep 28, 2023
9 of 12 checks passed

joelit deleted the issue1436-vocab-search-template branch September 28, 2023 11:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vocabulary search list Twig-templates #1470

Vocabulary search list Twig-templates #1470

joelit commented May 31, 2023 •

edited

Loading

sonarqubecloud bot commented May 31, 2023

codecov bot commented May 31, 2023 •

edited

Loading

joelit commented Aug 24, 2023

osma left a comment

osma commented Aug 31, 2023

osma commented Sep 19, 2023

joelit commented Sep 21, 2023

osma left a comment

joelit commented Sep 27, 2023

osma left a comment

osma Sep 27, 2023

joelit Sep 28, 2023

sonarqubecloud bot commented Sep 28, 2023

osma left a comment

Vocabulary search list Twig-templates #1470

Vocabulary search list Twig-templates #1470

Conversation

joelit commented May 31, 2023 • edited Loading

Reasons for creating this PR

Link to relevant issue(s), if any

Description of the changes in this PR

Known problems or uncertainties in this PR

Checklist

sonarqubecloud bot commented May 31, 2023

codecov bot commented May 31, 2023 • edited Loading

Codecov Report

joelit commented Aug 24, 2023

osma left a comment

Choose a reason for hiding this comment

osma commented Aug 31, 2023

osma commented Sep 19, 2023

Layout issues

joelit commented Sep 21, 2023

osma left a comment

Choose a reason for hiding this comment

joelit commented Sep 27, 2023

osma left a comment

Choose a reason for hiding this comment

PR title

global-search.inc

General page layout

Extra language tags & untranslated types

Extra whitespace and padding

Possibly unnecessary functionality in the template

Cypress tests

osma Sep 27, 2023

Choose a reason for hiding this comment

joelit Sep 28, 2023

Choose a reason for hiding this comment

sonarqubecloud bot commented Sep 28, 2023

osma left a comment

Choose a reason for hiding this comment

joelit commented May 31, 2023 •

edited

Loading

codecov bot commented May 31, 2023 •

edited

Loading