search

samvera / iiif_manifest

Other
10 stars 10 forks source link

Support for a 'sequence_rendering' method #3

Closed ghost closed 7 years ago

ghost commented 7 years ago

I'm working on Hyku 1372 and wanted a way of adding a 'rendering' element into the sequences.

I came up with #manifest_extras as a default place to extend iiif_manifest with extra elements. #manifest_extras would return a hash, which can be empty.

For the current implementation, if the #manifest_extras hash contains :sequence_rendering [array of rendering hashes] these would be picked up by the sequence_builder and added into the manifest.

I thought this could also be extended easily, eg. :metadata could be provided to add metadata at manifest level.

jcoyne commented 7 years ago

@geekscruff Is it possible that manifest_extras could be optional? Could you test for respond_to?(:manifest_extras) prior to calling it or provide a default implementation that can be overridden?

I'd also like to see @tpendragon @escowles @jpstroop @mejackreed or @cbeer have a look at this too.

Thanks for your work on this @geekscruff!

ghost commented 7 years ago

thanks @jcoyne ... adding respond_to? sounds good to me _ I was a little concerned about breaking the gem for others, and adding complexity some folks wouldn't need. I've made that change.

tpendragon commented 7 years ago

Sorry, I missed this. I don't feel great about manifest_extras - it feels like a catch-all solution that could quickly get nasty. I think I'd prefer if for the individual use cases for additional items to add to the manifest that we figure out the right interface. The use case here seems to be rendering - can we just add a rendering method as a MAY?

ghost commented 7 years ago

@tpendragon Now I've made it MAY, I prefer this solution. I'll make the changes.

tpendragon commented 7 years ago

@geekscruff Thanks!

ghost commented 7 years ago

@jcoyne I've made those changes, plus changed from 'manifest extras' to 'sequence rendering' method.

ghost commented 7 years ago

@jcoyne @tpendragon is this looking OK now? I went with '#sequence_rendering' to couple the method to where it's added, 2nd commit should be just the merge from master, 1st is my changes. Thanks!

tpendragon commented 7 years ago

I'm cool with it. @jcoyne ?