-
Notifications
You must be signed in to change notification settings - Fork 10
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
Add thumbnail property #79
Conversation
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.
I think we could rename the thumbnail_builder_factory
to thumbnail_builder
.
The design pattern of a factory is that you call the factory object to get you the class you want to instantiate based on the parameters passed to the factory.
In this case, we're passing in the specific class and directly call .new
.
display_content.try(:iiif_endpoint) | ||
end | ||
|
||
def image_service_builder |
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.
Is there a problem if the image_service_builder_factory receives nil
?
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.
looks like down the line, it will affect it here: https://github.com/samvera/iiif_manifest/blob/main/lib/iiif_manifest/v3/manifest_builder/image_service_builder.rb#L12-L14
are you thinking lonely operators for those three lines?
spec/lib/iiif_manifest/v3/manifest_builder/canvas_builder_spec.rb
Outdated
Show resolved
Hide resolved
spec/lib/iiif_manifest/v3/manifest_builder/canvas_builder_spec.rb
Outdated
Show resolved
Hide resolved
spec/lib/iiif_manifest/v3/manifest_builder/canvas_builder_spec.rb
Outdated
Show resolved
Hide resolved
spec/lib/iiif_manifest/v3/manifest_builder/canvas_builder_spec.rb
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 @kirkkwang, this looks like great work! I've always found the dependency injection pattern used in iiif_manifest
to be really hard to wrap my head around and work with but you've been able to figure it out. I added a few higher level comments to start with before doing a real close review. Thanks again for moving this gem forward!
@@ -54,8 +56,9 @@ def setup_manifest_from_record(manifest, record) | |||
manifest.rendering = populate_rendering if populate_rendering.present? | |||
homepage = ::IIIFManifest.config.manifest_value_for(record, property: :homepage) | |||
manifest.homepage = homepage if homepage.present? | |||
apply_thumbnail_to(manifest) |
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.
Could this be taken from the child canvas instead of being computed again? Maybe something like manifest.items.first&.thumbnail
would be sufficient? Although it might not be populated until after canvas_builder.apply(manifest.items)
on line 21.
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.
I wish it was that easy haha, here's a PR that I think would achieve this. My approach was when the canvas thumbnail
gets set, I set the manifest thumbnail
as well, if it doesn't exist already. To achieve that, I had to pass manifest in and that affected a lot more files than originally anticipated.
Actually I feel better about it now after taking a few days away from it. I've updated the way manifest thumbnails get set and fixed the spec. This also lets Collection manifests to have multiple thumbnails (which what UTK does in their implementation, not sure of other places however 🤔)
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.
Another idea i had was would it be better to make the thumbnail on the manifest configurable?
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.
Yes, that's a great idea. I think like with canvases iiif_manifest
should allow configuring the thumbnail and can fall back to using the first canvas's thumbnail if not already set.
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.
That sounds good, can you merge in your branch and I'll take care of linting and then I can work on the configuration?
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.
I merged it in and resolved the rubocop issues.
…t-from-record-property-builder remove apply thumbnail to manifest and fix specs
…to-manifest-from-record-property-builder Revert "remove apply thumbnail to manifest and fix specs"
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.
Looks great! Thanks for your huge effort on adding this feature! 🎉
Closes #34
Summary
This PR will add a
thumbnail
property to eachCanvas
item and also add one to the base manifest.ThumbnailBuilder
and specthumbnail
propertyREADME
Approach
My approach was coming from using a Hyku
4.1.0
with Hyrax3.4.1
which uses RIIIF1.7.1
as the image server and Universal Viewer3.1.4
. I followed the manifests from The University of Tennessee, Knoxville as an example. I am using a scaled down version of the IIIF image itself as the thumbnail instead of a pre-generated image for now. Also, since UV works fine with displaying thumbnails with a version 2 manifest, the focus was on version 3 to address this issue: UniversalViewer/universalviewer#102. I did my best to follow the existing pattern.Screenshots