Conversation
|
c152756 to
e09613c
Compare
|
69bacea to
4b2eeb7
Compare
UnniKohonen
left a comment
There was a problem hiding this comment.
General layout/usability notes:
- All vocabularies should be the first option in the vocab dropdown menu
- There should maybe be some padding/margins inside the vocab dropdown menu
- The button closing the search bar is not implemented
- Checkboxes should have custom styling
- Checking/unchecking vocabs is annoying because the dropdown moves
- There is some lag in the animation for me when opening/closing the menu
UnniKohonen
left a comment
There was a problem hiding this comment.
The new vocab list looks great! I forgot to mention this in the last review but the vocab list is missing the option for all vocabularies, is it supposed to be included in Skosmos 3.0/this PR?
Some small nitpicks about the layout:
- In the layout guide, the unchecked checkboxes have a light background and a dark border (but the guide is for Finto and not Skomos so it's probably OK)
- The magnifying glass button is not a square
|
|
SonarQube is complaining about the repetitive nature of the two dropdown menus in the search bar, wishing for a generic parameterized implementation that renders the different dropdown menus. In this case where we should ever have just the two menus for vocab and language, I am not sure getting rid of the repetitive code would improve the readability or the maintainability of the code. |
osma
left a comment
There was a problem hiding this comment.
Looks pretty good. Obviously some functionality is still missing (e.g. proper i18n) but it's important to get the core merged so that other work can continue.
The only thing I would change before merging this PR is the handling of URL parameters: handle the parameters for the search result page separately from the REST API search method parameters. Suggestions in the comments.
Also, I suggested adding a couple of trans filters in the Twig templates, so we don't forget that they need to be translated. You can simply click to accept my suggestions if those are OK.
Other things I noticed that probably need to be considered later:
- Handle translations for the UI messages, both in JS and in Twig templates.
- I tested what happens if you don't select any vocabularies. Then the
vocabparameter is left empty and search is performed over all vocabularies. Not a bad outcome, but maybe a bit surprising... Another option would be to not perform a search until the user has made a selection. - If I don't select a language, the
clangparameter will be set tonulland the search always returns an empty result. Again, I think it would be better to not perform a search until the user has chosen a language. - On the search result page, the global search bar is available but initially hidden. I think it would be better to always show it on the search result page. Also, it should remember the selections and the keyword.
- Minor detail: When selecting multiple vocabularies, they are listed as e.g. "YSO YKL JUHO Metatietosanasto", which may be a bit confusing; I suggest separating multiple values with commas instead, like in Skosmos 2.
|
The requested changes were applied. I'll make sure the related epic contains the updated specifications, or the request for discussion thereof. I ended up splitting the language updating mechanism from the language parsing mechanism for ease of maintenance. |
osma
left a comment
There was a problem hiding this comment.
Looks good.
I found a couple of minor CSS variable mishaps that perhaps could be fixed. Also, I'd like to avoid using Finnish language messages in the code, so I suggested changing the base messages to English for the benefit of non-Finnish testers of 3.0beta2. (If you change those, Cypress tests will need to be adjusted accordingly as well.)
|




Link to relevant issue(s), if any
Description of the changes in this PR
Known problems or uncertainties in this PR
Not implemented in this PR:
Checklist
.sr-onlyclass, color contrast)