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

L is not defined in leaflet-iiif #2961

Open
jcoyne opened this issue Mar 8, 2023 · 9 comments
Open

L is not defined in leaflet-iiif #2961

jcoyne opened this issue Mar 8, 2023 · 9 comments

Comments

@jcoyne
Copy link
Member

jcoyne commented Mar 8, 2023

When loaded as a module. It may depend on L as a constant (via Leaflet)

https://github.com/projectblacklight/spotlight/blob/main/vendor/assets/javascripts/leaflet-iiif.js#L7

The original is in @mejackreed's repo: https://github.com/mejackreed/Leaflet-IIIF

@mejackreed
Copy link
Contributor

Has an absolute hard dependency on Leaflet

@jcoyne
Copy link
Member Author

jcoyne commented Mar 9, 2023

Hi @mejackreed 👋 I'm wondering it it can be patched to import L from "leaflet-src.esm.js" rather than expectingL to be defined globally?

@mejackreed
Copy link
Contributor

For the fork here in Spotlight, sure? Leaflet plugins usually follow a pattern to extend L from an expected global existence. Maybe there are other patterns out there to handle this?

Also, I have no understanding anymore about how Rails expects this to work 😉

// leaflet-iiif importer

import L from "leaflet-src.esm.js";
import leaflet-iiif

@jcoyne
Copy link
Member Author

jcoyne commented Mar 9, 2023

@mejackreed Using ESM modules seems to be the path forward for Leaflet:

import leaflet/dist/leaflet-src.esm.js explicitly instead to take advantage [of modules]; ESM by default will come in v2

https://github.com/Leaflet/Leaflet/releases/tag/v1.9.2

@jcoyne
Copy link
Member Author

jcoyne commented Mar 9, 2023

Here is a PR that would resolve this: mejackreed/Leaflet-IIIF#93

@taylor-steve
Copy link
Contributor

Noting that:

  • Justin's work here makes Leaflet-IIIF work with import maps. I've been using it in my exploratory BL8 builds in Spotlight and it works great for that. If we can't get it into the gem, we might need to make a decision (e.g., fork or apply the patch locally in Spotlight).
  • @cbeer pointed out that Leaflet-IIIF is likely only used for one or two purposes in Spotlight (e.g., image cropping). Now that we're willing to introduce breaking changes, another valid approach to this ticket could be to replicate/replace that functionality using a different approach and removing the dependency on Leaflet-IIIF.

@taylor-steve
Copy link
Contributor

Confirming it appears this is only used in Spotlight for the cropper in admin/crop.js.

It seems we could replicate this cropping tool using OSD or OpenLayers, but it'd be work. It might be worth waiting until we know more about sul-dlss/exhibits#357 on the off chance we need to support something beyond a fixed aspect ratio rectangle.

If the existing tool is sufficient, I'd suggest we wait until we're slightly further in the overall JS work.

@taylor-steve
Copy link
Contributor

@dnoneill suggested https://github.com/annotorious/annotorious which looks promising should we want to switch to OSD.

@taylor-steve
Copy link
Contributor

I think we may want to explore the replacement option for the cropper further. If it's on the order of ~1-2 days of work, it may be faster than trying to resolve all the quirks it is having in the Propshaft build.

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

No branches or pull requests

3 participants