Add thumbnail property #79
Conversation
There was a problem hiding this comment.
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.
Is there a problem if the image_service_builder_factory receives nil?
There was a problem hiding this comment.
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.
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.
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.
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.
Another idea i had was would it be better to make the thumbnail on the manifest configurable?
There was a problem hiding this comment.
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.
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.
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"
Closes #34
Summary
This PR will add a
thumbnailproperty to eachCanvasitem and also add one to the base manifest.ThumbnailBuilderand specthumbnailpropertyREADMEApproach
My approach was coming from using a Hyku
4.1.0with Hyrax3.4.1which uses RIIIF1.7.1as 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