Fix thumbnail panel search result display (resolves #1409)

[LlGC-jop]

Fixes #1409

Copies colour + image from gallery view component

Removes fixed width from the label which was breaking layout. Selector left commented out in case needed in future (see below)

I would recommend a CSS update of the thumbs view to use grid rather than floats at some point, but I've left it as-is for now.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
universalviewer	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 15, 2025 2:18pm

[LanieOkorodudu]

@LlGC-jop, I tested this fix and it works as expected. I can confirm on my end that it addresses the issue #1409. Thanks for your work, let's wait for @demiankatz input since he initially raised the issue.

[demiankatz]

Thanks, @LlGC-jop and @LanieOkorodudu. This looks good to me as well, but one question: does anyone know of a manifest that supports search and has very long page labels? I'd like to test these changes in 1-up and 2-up mode with the "truncate thumbnail labels" setting both on and off to see if any of those combinations introduce any problems... but I don't know of a manifest that has labels long enough to need truncation and which also supports search. I have examples of each individual thing, but not the two together. (You can see https://collections.st-andrews.ac.uk/762295/manifest for an example of a manifest with long labels, but it's not much help for testing here since it doesn't support 2-up mode or search).

[LanieOkorodudu]

@demiankatz, I wasn’t able to find a manifest on our end that includes a long label combined with the search function. If testing that scenario is important to confirm the fix, it might be helpful to share this PR in the general or community channel, perhaps another institution has a suitable manifest. We'll wait a day to see if any feedback comes in. Otherwise, this fix looks good to me and is good to merge. Just a suggestion from my side.

[demiankatz]

Thanks, @LanieOkorodudu, I agree -- let's give this a day and see if anyone finds anything. If not, we can at least confirm that this is a significant improvement for all common use cases, and if any new problems are discovered, we can address them as they come to light.

[LlGC-jop]

@demiankatz I'll have another look at this after the sprint call.

Using devtools, longer labels do break the layout a little, but it's no worse than before.

I can make some improvements to the layout css using grid, but avoided that initially so it wouldn't take too long. I'll tinker with it today and report back.

[LlGC-jop]

Long labels should be ok now. If you use devtools to put some lipsum in there it should flow sensibly.

I've made a fairly drastic change to the thumbnail layout so it uses grid now.

This initially broke RTL but I've fixed it, using the manifest below to test, as it has some pretty illustrated pages that make layout obvious :)

https://iiif.bodleian.ox.ac.uk/iiif/manifest/1d091ec9-33f8-42d9-a620-141dfc7b1816.json

Unfortunately it doesn't have a search service, not that I'd be able to test with arabic chars anyway!

[demiankatz]

Thanks, @LlGC-jop, this looks good to me overall.

One observation, which is not a huge problem but which might be worth investigating: when I toggle the "truncate thumbnail labels" option in the settings dialog box using the RTL manifest you shared, the thumbnails move slightly, like a margin or padding is being impacted somehow. When I do the same thing on the default LTR manifest, the setting does not cause any shifting of content. Is this a sign that there's a missing style somewhere to properly account for right-to-left formatting? I can live with it if there's no easy solution, but if it's something simple/obvious, we might as well straighten it out. :-)

[LlGC-jop]

@demiankatz

I'll look into it - the styling should be identical apart from a tweak to the grid settings but it's possible I've missed a margin/padding somewhere in the old css that needs removing.

[LlGC-jop]

Should be ok now.

Vertical spacing will still jump when a truncated word drops to line 2, but the horizontal should be static now.

[demiankatz]

Thanks, @LlGC-jop, this is definitely improved; however, there's just one more problem with the right-to-left layout. The first thumbnail should be right-aligned instead of left-aligned when in RTL mode.

Correct layout (from current dev branch):

Incorrect layout (from this branch):

[LlGC-jop]

Hi @demiankatz

It was an intentional change, based on the idea that the physical position of a RTL book is reversed, and having the cover be over on the left puts the spine in the correct position in the center.

Happy to revert, or discuss in the sprint call tomorrow if you want to get feedback from the others?

[demiankatz]

Thanks, @LlGC-jop! I had just assumed the existing code was correct and noticed that this was opposite of the existing code... but now that you've explained, I agree that the change here fixes a problem. I think this is ready to merge now. However, I notice that this PR targets dev; should it instead target release-4.2.0 so we can include it in the next RC? Please let me know if you need any help rebasing things (or if you think we should NOT rebase).

[LlGC-jop]

@demiankatz

sure let's change to release-4.2.0 - can you tell me how for future reference?

I found a dropdown at the top of the page when I click edit, but it warns about commits changing or being lost.

Is that the correct way to change the target?

[demiankatz]

@LlGC-jop, it depends on whether the original branch was forked off of release-4.2.0 or dev. If you started with dev, then changing the base branch in the PR will try to merge all changes from the dev branch back into release-4.2.0, not just the ones relevant to this PR.

If you're based on the dev branch, here's what I would suggest (assuming that your branch is fully up to date with the latest dev branch -- i.e. you've merged in latest changes).

git reset dev
git stash
git reset --hard release-4.2.0
git stash pop
git commit -m "Fix thumbnail panel search result display (resolves #1409)"
git push --force

That should flatten everything into a single commit built on top of the release-4.2.0 branch. Of course, if the stash pop doesn't work cleanly, it might not be that simple.

Any help? I'm happy to give this a shot on my end if it gives you any trouble on yours.

[LlGC-jop]

I'll give it a go @demiankatz and see how it goes - I should probably learn a bit more git at some point :)

[LlGC-jop]

I've run those @demiankatz but it doesn't seem to have changed the target.

Feel free to have a go - I had a quick google and github seems to suggest a rebase --onto : https://github.com/supercollider/supercollider/wiki/Rebasing-a-PR#rebasing-with-a-branch-other-than-develop

[demiankatz]

Thanks, @LlGC-jop -- I neglected to mention that after doing those steps, you can then manually edit the target in the PR. I've just done that, and the diffs look clean, so I think we're in good shape now!

[demiankatz]

I think this is all set now, but I'll wait until tomorrow's stand-up to merge, in case anyone else has further input in the meantime.

[LanieOkorodudu]

@LlGC-jop and @demiankatz, I tested this using the example manifest mentioned above. I'm just curious, was it intentional for the "Next Image" button to appear on the left? Is this due to how the material is structured?

[demiankatz]

@LanieOkorodudu, because Arabic is read from right to left instead of from left to right, the controls are intentionally reversed when that manifest is loaded.

[LanieOkorodudu]

@LanieOkorodudu, because Arabic is read from right to left instead of from left to right, the controls are intentionally reversed when that manifest is loaded.

Ah, okay, that makes sense. In that case, everything looks good on my end.

[rafeili]

I had a look at the changes in the code and ran - and tested - uv4 from this branch in localhost. Looks good.
As for the background image in src\content-handlers\iiif\modules\uv-shared-module\css\thumbs-view.less: Did you use a free Base64 image converter/encoder and if so, which one? Can we put information on this somewhere in the documentation for developers in order to facilitate reproduction/update of different background-images that need to be added in the CSS? By the way, we have some console issues regarding "Accessibility". Maybe something to have a deeper look into in some coming sprint in the future?
Thanks.

This fix can be merged.

[LanieOkorodudu]

I had a look at the changes in the code and ran - and tested - uv4 from this branch in localhost. Looks good. As for the background image in src\content-handlers\iiif\modules\uv-shared-module\css\thumbs-view.less: Did you use a free Base64 image converter/encoder and if so, which one? Can we put information on this somewhere in the documentation for developers in order to facilitate reproduction/update of different background-images that need to be added in the CSS? By the way, we have some console issues regarding "Accessibility". Maybe something to have a deeper look into in some coming sprint in the future? Thanks.

This fix can be merged.
@rafeili, We had an accessibility-focused sprint last year, where we implemented improvements such as better labelling, keyboard navigation, and more. That said, it would be great if you could share or raise any specific issues you're seeing on your end. I agree, if it requires a deeper look and further improvements, another sprint might be the best way to address it.

[LlGC-jop]

Happy for this to be merged.

@rafeili The data uri for the magnifying glass image comes from the gallery component's CSS (https://github.com/IIIF-Commons/iiif-gallery-component/blob/master/src/css/iiif-gallery-component.less) in order to make it consistent with the fully open panel.

In that file it's processed at compile time (I think) but I used DevTools to grab the resulting string because it avoids having an actual file.