use explicit manifest#1755
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@LanieOkorodudu is attempting to deploy a commit to the Universal Viewer Team on Vercel. A member of the Team first needs to authorize it. |
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @LanieOkorodudu -- see below for some questions and suggestions.
| const { BASE_URL } = require("../scripts/testBaseUrl"); | ||
|
|
||
| // Cookbook manifest for viewer control tests | ||
| const COOKBOOK_MANIFEST = |
There was a problem hiding this comment.
Maybe we should give this a more specific name, since we'll likely have more cookbook manifests in the future -- perhaps COOKBOOK_BOUND_MULTIVOLUME_MANIFEST?
| "https://iiif.io/api/cookbook/recipe/0031-bound-multivolume/manifest.json"; | ||
|
|
||
| // PDF manifest for PDF-specific behaviour | ||
| const PDF_MANIFEST = |
There was a problem hiding this comment.
Maybe this should be PDF_MULTI_FILE_MANIFEST -- since the edge case this manifest was provided to test is a set of multiple PDF files in a single manifest. One of the things we should confirm when loading the manifest is that the left sidebar appears with multiple files listed. (If we load a single-PDF manifest, the left sidebar should NOT appear).
| const viewerUrl = (manifestUrl) => { | ||
| const separator = BASE_URL.includes("?") ? "&" : "?"; | ||
|
|
||
| return `${BASE_URL}${separator}manifest=${encodeURIComponent(manifestUrl)}`; |
There was a problem hiding this comment.
Does this really work? I thought the manifest argument had to be part of the hash parameters, not part of the base query parameters... in other words, https://www.universalviewer.dev/?manifest=https://digital.library.villanova.edu/Item/vudl:294631 does not work, but https://www.universalviewer.dev/#?manifest=https://digital.library.villanova.edu/Item/vudl:294631 does work. Note the #? instead of just ?.
There was a problem hiding this comment.
It did appear to work when I tested yesterday, but you are right that the manifest parameter should probably be part of the hash parameters rather than the base query parameters. I’ll update viewerUrl to use #?manifest= instead. Thanks
2 participants
Description of what you did:
This PR updates the test to explicitly specify manifest for different test scenarios instead of relying entirely on the default manifest.
Changes made
BASE_URL, since features such as gallery/two-up view are only available there.The PDF test currently verifies that:
I also increase the
jest.setTimeoutto 60000 to accommodate manifest loading that may take more time during PDF and cookbook manifest tests.