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

Path to uv.html can now be set with options.embedTemplate #1254

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

crhallberg
Copy link
Contributor

@crhallberg crhallberg commented Dec 12, 2024

By moving /uv.html to the template, I was able to split the appUri and the hash parameters. You can now pass in a custom iframe template to set the path to uv.html.

I also utilized URLSearchParams to make the hash parameters easier to understand and update.

For example, in VuFind:

{
  "modules": {
    "shareDialogue": {
      "options": {
        "embedTemplate": "<iframe src=\"{0}/themes/vudiglib/assets/uv-4.1.0-rc1/uv.html#?{1}\" width=\"{2}\" height=\"{3}\" allowfullscreen frameborder=\"0\" title=\"{4}\"></iframe>"
      }
    }
  }
}

Admittedly, this is also possible with a RewriteRule.

RewriteRule ^/uv.html /themes/vudiglib/assets/uv-4.1.0-rc1/uv.html [R=301,L]
  • Config uvAppPath (default to uv.html).

Copy link

vercel bot commented Dec 12, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
universalviewer ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 16, 2024 3:17pm

Copy link
Contributor

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks, @crhallberg, this is full of good ideas, but I have some suggested refinements... :-)

src/uv-iiif-config.json Outdated Show resolved Hide resolved

return appUri + "/uv.html";
const port = window.location.port ? `:${window.location.port}` : "";
return `${window.location.protocol}//${window.location.hostname}${port}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than moving /uv.html from here into the embed template, would it be better to create a new config setting like uvAppPath that defaults to /uv.html? Then we wouldn't have to change the embed templates as much, and other code looking for the full path to uv.html would also work correctly.

I wonder if it would make sense to split this PR in two: one to do the hash parameter simplification, since I think that is a useful improvement to readability, and one to add the new config setting. Another advantage to this approach: we wouldn't have to change the number of placeholders in the embed template, which would better maintain backward compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea. I think that getAppUri should not include /uv.html but if I'm misunderstanding the usage...

Copy link
Contributor

@demiankatz demiankatz Dec 16, 2024

Choose a reason for hiding this comment

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

I think it's okay for getAppUri to include /uv.html -- that is part of the URL of the UV application itself. I could imagine it being useful to get the URL of the path to the application without the filename, but I think that would be a separate method that doesn't exist yet (presumably because it hasn't been needed yet). I'd be inclined to keep method names and behaviors as they are, and refactor in future if a need arises.

Co-authored-by: Demian Katz <[email protected]>
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.

2 participants