Conversation
osma
left a comment
There was a problem hiding this comment.
Looks good! A few comments:
Visual style
There is no padding on the left side of the ellipsis (...) and sometimes this leads to visual artefacts that look a bit broken:
It would be better to have some left padding, as in Skosmos 2, so that the ellipsis doesn't visually merge with the text.
Even better would be some kind of transparent fade-out effect, as in this picture (drawn using Gimp):
Pointer style
I noticed that the mouse pointer becomes a cursor (vertical bar) when hovering above the link, not a pointer (hand with finger) as I'd expect. I think this is some CSS rule that applies to the whole search result list, because the same happens when hovering over prefLabels in other languages (e.g. "dog parks" in above pictures). I think the correct thing to do here would be to show a regular arrow cursor over "dog parks" (it's not clickable after all), and a pointer cursor when hovering over the "click for more" link.
Tooltip
I think it would be useful to show a tooltip (just a plain link title would do) that explains what happens if you click on the link. For example "expand to show all items" or something along those lines.
Use of .style attribute
I noticed that the JS code sets element styles directly using the .style attribute. I think it would be better to set a specific class for the link elements and define the styles using CSS instead - that's easier to maintain in the future. In some cases it might be helpful to use Bootstrap-defined styling classes such as p-0 for padding: 0.
|
I implemented all suggestions except the tooltip text. Is the translation infrastructure ready for it in case of plain JS functions, or should I implement a placeholder text? |
|
There are now some JS problems that cause CI tests to fail. SonarCloud also complains about an unused JS variable. |
Ah, sorry. Fixed! |
|
The tooltip for "show more" is a good idea, but it really wants a ready and documented translation infrastructure for vanilla JS-components. I will note it in the relevant epic-issue. |
b692f02 to
7526a32
Compare
osma
left a comment
There was a problem hiding this comment.
There are still some .style settings that could perhaps be moved to CSS, see comments.
The fade-out effect looks quite good but I think there's too much overlap between the ellipsis (...) and the fading text so the result looks a bit messy:
I think the text should fade out completely before the ellipsis, maybe with a few pixels of empty (white) space in between the two.
JS. Tweaked the linear gradient fade.
|






Link to relevant issue(s), if any
Closes #1528
Description of the changes in this PR
Implementation for truncating long search results
Checklist
.sr-onlyclass, color contrast)