Skip to content
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

IIIF viewer choice #3360

Merged
merged 3 commits into from
Oct 31, 2018
Merged

IIIF viewer choice #3360

merged 3 commits into from
Oct 31, 2018

Conversation

cjcolvar
Copy link
Member

Pulls in work from https://github.com/samvera-labs/hyrax-iiif_av to generalize the IIIF viewer support to any IIIF viewer and not assuming Universal Viewer. In the future other viewers could be added to core Hyrax but this PR at least allows a downstream app the ability to customize the IIIF view partial without overriding. This is helpful for more experimental viewers like the Avalon react player for IIIF Presentation 3.0 AV manifests. Possible documentation for how to use a custom IIIF viewer is below.

This PR has some view partial changes so I'd hope that it would be considered for Hyrax 3.0.

@samvera/hyrax-code-reviewers

Adding a custom IIIF viewer

Create a new file in your Hyrax app at app/views/hyrax/base/iiif_viewers/_my_iiif_viewer.html.erb:

<h3>My IIIF Viewer!</h3>
<a href=<%= main_app.polymorphic_path([main_app, :manifest, presenter], { locale: nil }) %>>Manifest</a>

Override #iiif_viewer in your work presenter app/presenters/hyrax/generic_work_presenter.rb:

def iiif_viewer
  :my_iiif_viewer
end

@cjcolvar cjcolvar changed the title Iiif viewer choice IIIF viewer choice Oct 30, 2018
Copy link
Contributor

@no-reply no-reply left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Could we get an @example of replacing the viewer with a (real or imagined) alternative?

I'm hoping @kdid can give a quick review to this as well.

representative_id.present? &&
representative_presenter.present? &&
representative_presenter.image? &&
Hyrax.config.iiif_image_server? &&
members_include_viewable_image?
end

alias universal_viewer? iiif_viewer?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 nice work!

alias universal_viewer? iiif_viewer?
deprecation_deprecate universal_viewer?: "use iiif_viewer? instead"

# Override this method to declare a different iiif viewer for your work type
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we just document that this method returns the viewer type as a symbol? I fear that saying "override" encourages folks to reopen this class, while inheriting it or implementing its interface would be better.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WorkShowPresenter is inherited by the work presenter generated as part of a work type. I was expecting overrides to happen at that level in the downstream app's work presenter. What do you think would be clearer language to suggest that pattern? Do you think the @example you suggest would serve this purpose?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know lots of folks reach straight for monkey-patching, and I'd like to move docs to language that doesn't invite that interpretation.

@no-reply
Copy link
Contributor

This seems nice and clean, and I'm not sure it requires a 3.0.0. What do you think about releasing this in the 2.x series as well?

@no-reply no-reply added this to the 2.x series milestone Oct 30, 2018
@cjcolvar
Copy link
Member Author

The failing test is coming from my use of the same changes as #3341 without changing the tests. I'm inclined to merge that PR then rebase this one to pick up the fix.

@cjcolvar
Copy link
Member Author

@no-reply I was hesitant to ask for this in 2.0 since there are changes to views even if the behavior stays the same. But maybe it is backwards-compatible in that if you have overridden one of the partials I changed then nothing should happen as a result of upgrading to this code.

This follows the pattern of #media_display to allow a work type to declare the iiif viewer it wants displayed and then use some helpers to render it.
Based on hyrax-iiif_av (which was extracted from avalon-bundle).
@kdid
Copy link
Contributor

kdid commented Oct 30, 2018

👍 This looks good!

@no-reply
Copy link
Contributor

But maybe it is backwards-compatible in that if you have overridden one of the partials I changed then nothing should happen as a result of upgrading to this code.

That's my interpretation. These changes seem carefully made, and can't see a way they could reasonably break expectations of a downstream app.

Copy link
Contributor

@no-reply no-reply left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@afred afred merged commit fa8f81b into master Oct 31, 2018
@afred afred deleted the iiif_viewer_choice branch October 31, 2018 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants