Add islandora_mirador submodule #10
Conversation
Yes, if you don't set a context, then you will see a white screen for all pages. But, if you restrict the viewer to only show on Paged Content nodes, it works as described above. Nice work. Thank you. |
|
I'll see if there is anything else I can come up with to avoid a white screen without a context to limit the application of the block because that doesn't seem great to get an automatic white screen when you enable the module |
|
I added the default context to the PR so this should avoid the white screen issue. |
|
@elizoller Any progress on this? |
|
I need to revise this PR before it can be tested. Hopefully in the next week. |
|
@Islandora-CLAW/committers This should be ready for re-review |
|
Just dropping a note that I have not been able to acheive the command line magic required to pull this in and test it locally, so I think I have to bow out of the group of testers. Sorry! |
|
Likely should spun off another ticket but should Not sure how this would lineup with what other modules are doing @dannylamb @Islandora/8-x-committers, for reference: https://www.drupal.org/docs/drupal-apis/services-and-dependency-injection/services-and-dependency-injection-in-drupal-8#s-using-dependency-injection. |
| mirador: | ||
| version: 1.x | ||
| js: | ||
| js/mirador.min.js: {minified} |
There was a problem hiding this comment.
More of a discussion point than anything actionable but what's the undertook approach in Islandora in general for relying on external libraries? Seems Openseadragon at one point pointed out to Git and there's also the inflight CDN pull: https://github.com/Islandora/openseadragon/pull/26/files.
There was a problem hiding this comment.
did that cdn exist or did islandora put it there? it looks like there is https://www.jsdelivr.com/package/npm/mirador - i could probably swap that out if that's the way we want to go
@dannylamb
There was a problem hiding this comment.
That CDN exists I believe. I'm not sure what the defacto Drupal 8 approach is for pulling in packages tbh. One option would be to do something like https://www.bounteous.com/insights/2020/04/22/guide-loading-external-javascript-drupal/?lang=en-ca and utilize https://asset-packagist.org/.
However, not sure entirely this is the "correct" thing to do and may add more work on top of this.
There was a problem hiding this comment.
i changed it to the cdn version in 4a8a86b - what do people think of that?
There was a problem hiding this comment.
I have no preference around version or how things should be pulled in, was trying to get @dannylamb's attention to what we may want to do (know this discussion has come up on committers before).
| * The DOM element that represents the Singleton Instance of this class. | ||
| * @type {string} | ||
| */ | ||
| var base = 'mirador'; |
There was a problem hiding this comment.
Maybe something to note as an improvement / future use case. Would we want to bring this in as a setting and look to do something like https://github.com/Islandora/openseadragon/pull/26/files to support having multiple Miradors rendered on a page? In @seth-shaw-unlv's case he had a carousel where the viewers were being rendered.
There was a problem hiding this comment.
can i assume you mean this one Islandora/openseadragon#28 ? i'll see if i can to do the same for this.
There was a problem hiding this comment.
Whoops ya 28. I mean don't necessary need to do it in this ticket just noting for a future improvement.
There was a problem hiding this comment.
seems silly not to have it...
There was a problem hiding this comment.
alright... i think i might have to leave this as is and let someone else tackle it. my first several attempts didn't work as expected
There was a problem hiding this comment.
i am not entirely sure mirador allows this. i saw a reference that said multiple miradors in a single page would require iframes, but they may pertain only to older versions. so for now i enforced unique HTML ids, so some of the groundwork is at least laid.
modules/islandora_mirador/src/Plugin/Field/FieldFormatter/MiradorImageFormatter.php
Outdated
Show resolved
Hide resolved
|
@jordandukart The issues that |
|
Forgot to mention on my review, functionality wise this totally works and is a great contribution from @elizoller! Code style things are the only things that stood out to me. |
|
Keen to try this out but it doesn't apply as a patch on top of |
|
@kayakr I don't see any merge conflicts off of 8.x-1.x? |
|
@jordandukart I was trying to apply the pull request as a patch on 8.x-1.x and composer says the patch won't apply... |
|
Chatted with @elizoller in Slack and his changes since the last bit of review uncovered a bug with the implementation. That is with how Slack conversation for posterity and more details: https://islandora.slack.com/archives/CM5PPAV28/p1598647657104600. |
…e field value to the node - this implies a media is only attached to a single node.
jordandukart
dismissed
dannylamb’s stale review
Outdated and already addressed.
|
This looks good to me, going to let this percolate for 24 hours and if no-one else chimes in will merge tomorrow, thanks for the contribution @elizoller! |
What does this Pull Request do?
Adds Mirador 3 JS and a Mirador block with a configuration for the manifest URL.
What's new?
How should this be tested?
drush migrate:import islandora_defaults_tags)config/install/context.context.openseadragon_block.ymlandconfig/install/views.view.iiif_manifest.ymland all of theconfig/install/core.entity_view_display.node.islandora_object*files (for all display modes of the islandora_object content type) (my preferred method for importing just a few configs is copy them to a directory like/var/www/html/drupal/config/onetimeand then rundrush config:import --partial --source /var/www/html/drupal/config/onetimeInterested parties
@Islandora-CLAW/committers