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

Cdn #26

Merged
merged 5 commits into from
Nov 20, 2020
Merged

Cdn #26

merged 5 commits into from
Nov 20, 2020

Conversation

dannylamb
Copy link
Contributor

GitHub Issue:

What does this Pull Request do?

Uses a CDN for the openseadragon JS and images, which simplifies deployment. One less step to get the module up and running.

What's new?

Changed the library definition so that it points to a CDN for openseadragon. Plus changed the prefixUrl config for the JS to use the CDN as well, so the button images for the player are loaded over https.

How should this be tested?

  • Pull down this code
  • Delete web/sites/all/assets/vendor/openseadragon
  • drush cr
  • Go visit a page with the openseadragon viewer and if you watch it with the developer console, you should see requests for the viewer and its images going to CDNJS instead of your site.

Additional Notes:

This started out as me wanting to ship openseadragon with composer, but there's no "community way" to do that yet, and it involves a lot of consistency across root composer files for all Islandora sites using openseadragon. You can't just do it from the module's composer.json. So I did this. Maybe it's useful to someone?

Interested parties

@Islandora/8-x-committers

@dannylamb
Copy link
Contributor Author

If folks don't have objections to this approach, I'll update the ansible and ISLE stuff appropriately afterwards 🔥 🔥 🔥

@elizoller
Copy link
Member

@dannylamb do you still need a reviewer on this?

@dannylamb
Copy link
Contributor Author

There's still work to do here. I've got to figure out how to programatically get the version from the library config. I made an attempt a week or so ago and it thwarted me.

@nigelgbanks
Copy link
Member

This fixes the problem I've had with displaying openseadragon on subsites! 👍

@elizoller
Copy link
Member

@dannylamb i just tested this and it worked wonders for an issue i was having with multisite - is it waiting for anything or can i go ahead and review + merge?

Copy link
Member

@bseeger bseeger left a comment

Choose a reason for hiding this comment

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

This looks great and fixed the issue I was seeing.

@bseeger bseeger merged commit 6c9ae44 into 8.x-1.x Nov 20, 2020
@dannylamb
Copy link
Contributor Author

Let's move it in and we'll do a follow up to fix it. Right now, if you want to update the library, you'll have to touch up 3 or 4 spots. I could figure out how to make it a single variable you have to touch up, but it gave me grief last i tried.

@dannylamb
Copy link
Contributor Author

Ah, @bseeger ++!!!!

@bseeger bseeger deleted the cdn branch November 20, 2020 20:46
@dannylamb
Copy link
Contributor Author

Love it when i'm ninja'd by a merge!

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.

Deploy openseadragon js with composer
4 participants