-
-
Notifications
You must be signed in to change notification settings - Fork 184
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extending the thumbnail label to reveal more text #758 #950
base: dev
Are you sure you want to change the base?
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
it looks like the label span isn't filling the whole width of its containing element? Might make things easier to read. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @Saira-A. I don't claim to have done a particularly thorough review, but a few small suggestions below.
We should talk about code reviews and PR handling at the next Community Call, to see what strategy we can come up with to keep things moving while @edsilv is busy elsewhere. I unfortunately don't have a lot of time to keep on top of UV things, but I'll at least try to help a little when I can. :-)
src/content-handlers/iiif/modules/uv-contentleftpanel-module/GalleryView.ts
Outdated
Show resolved
Hide resolved
src/content-handlers/iiif/modules/uv-dialogues-module/SettingsDialogue.ts
Outdated
Show resolved
Hide resolved
@Saira-A, I also think some more work may be needed to support this functionality in extensions other than OpenSeadragon. At least, when I tested this with a multi-PDF manifest, the labels did not expand. You can see this here: |
I've done it so that when thumbnails are one up, they're extended by default as there's more space for them anyway so now the checkbox only affects two-up view |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See below for a few more thoughts and minor nitpicks.
Beyond that, though, there seems to be a bigger issue. I definitely like what you've done with 1-up labels, but it seems that 2-up mode has somehow gotten broken in the process. When I look at the original test case, it comes up in 1-up mode and I don't see a way to switch it to 2-up.
Here's a link for convenience:
src/content-handlers/iiif/modules/uv-contentleftpanel-module/ContentLeftPanel.ts
Outdated
Show resolved
Hide resolved
src/content-handlers/iiif/modules/uv-dialogues-module/SettingsDialogue.ts
Outdated
Show resolved
Hide resolved
I think this happened prior to my commit? I went back to the docs branch and it has the same issue, and there's no icons to switch between one and two up, yet other manifests look fine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed on today's steering group call, if you can make the change listed below in combination with reverting the latest change to the settings label (so it says "Truncate Thumbnail Labels" instead of "Show Full Thumbnail Labels"), then I think we'll have consistency between the settings name, the control label and the behavior, and we can avoid changing the default behavior. That should get this to a state where it can be merged!
src/content-handlers/iiif/modules/uv-shared-module/css/thumbs-view.less
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @Saira-A, this is great. As far as I can tell, everything is now working as expected, matching the behavior we discussed at the last Steering Group meeting.
I pushed up a few minor commits to adjust whitespace issues that were introducing unnecessary diffs in this branch, just to improve the readability of the final commit.
I did notice one strange issue related to the Aleph extension, but I think that dealing with it is likely beyond the scope of this PR. I'll just leave this open a little longer in case anyone else can offer insights. Hopefully we can push the merge button on this at the next Community Call if not sooner.
I really appreciate your work on this, as I believe this solves an issue that has been annoying to us at Villanova as well. Please let me know if I can be of any further assistance with anything!
@@ -199,6 +199,7 @@ | |||
"clickToZoomEnabled": "Mouse Click To Zoom", | |||
"pagingEnabled": "Two Page View", | |||
"reducedMotion": "Reduce motion (disables animations)", | |||
"truncateThumbnailLabels": "Truncate Thumbnail Labels", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any idea why this file includes raw text instead of placeholder strings like $truncateThumbnailLabels
? Is this something that needs to be fixed in a follow-up PR? (I'm not sure who actually uses this extension at this point...)
Thanks for resolving the conflicts, @Saira-A! I see that you removed the tests; did they stop working for you? I think it might be worth trying to revive them, though we might need to figure out the most appropriate place to put them. Maybe something to discuss on a future dev call. |
I removed the tests since the test suite doesn't run on the laptop I'm using and there have been other changes to the test file so I can't tell if they still work, but I still have them saved and we're getting new laptops soon so I'll try to put them back then |
Makes sense! Let me know once everything is ready and I can help test (and we can perhaps discuss the best placed to put the tests, since I seem to recall that when we looked at this before, we found there were a few possible approaches supported by the current setup... and it would probably be ideal to have a separate test file for each discrete feature). |
Issue #758