-
-
Notifications
You must be signed in to change notification settings - Fork 199
Issue 1503, fix continuous behavior #1519
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
base: dev
Are you sure you want to change the base?
Issue 1503, fix continuous behavior #1519
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
These fixes look great, @jamesmisson - no notes other than I agree about the positioning of the navigation :) Thank you for your work on this. |
Thanks @jamesmisson , for addressing issue #1503 . I know this is still a work in progress, so totally understand there’s more to refine. Since you mentioned the left panel being hidden for the thumbnail, I also noticed that the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @jamesmisson, this is a big improvement over being completely broken. :-)
What do you think needs to happen to get this out of draft mode?
From my perspective, there are a couple of code cleanup things (see below) but I'd otherwise be happy to accept this as-is in the spirit of incremental progress. If you want to further improve the UI before merging, though, that's fine too.
Please let me know if I can do more to help things along!
// stretch navigator, allowing time for OSD to resize | ||
// setTimeout(() => { | ||
// if (this.extension.helper.isContinuous()) { | ||
// if (this.extension.helper.isHorizontallyAligned()) { | ||
// const width: number = | ||
// this.$viewer.width() - this.$viewer.rightMargin(); | ||
// this.$navigator.width(width); | ||
// } else { | ||
// this.$navigator.height(this.$viewer.height()); | ||
// } | ||
// } | ||
// }, 100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this commented-out code actually needed for future reference, or is this an abandoned experiment?
Range, | ||
} from "manifesto.js"; | ||
import { ViewingHint } from "@iiif/vocabulary/dist-commonjs/"; | ||
// import { ViewingHint } from "@iiif/vocabulary/dist-commonjs/"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove this now?
This still needs work and lots of testing so I'll leave it in draft.
This addresses #1503, where some Presentation v2 'continuous' manifests, and all Presentation v3 continuous manifests, weren't displaying as continuous scrolls (i.e. all canvases stitched together).
The problem with the v2 manifests supplied in the issue was that UV was only checking for continuous viewingHints on the sequence level, not manifest level.
I've updated the 'isContinuous' method in Manifold to check both viewingHint (v2) and behavior (v3), and both range/sequence level, and manifest level: IIIF-Commons/manifold#64. So this PR can just use isContinuous to check and display all the canvases at once (rather than viewingHint as before).
This resulted in some glitches in the viewport navigation window (OSD's little map of the whole canvas in the bottom right). This was caused by some code in OpenSeadragonCenterPanel.ts that deliberately resized the viewport navigation for continuous manifests, presumably to make the long scroll format fit more neatly into a long thin viewport. I've rewritten this resizing stuff (currently the minimum and maximum widths/heights are hard-coded).
I've also moved the viewport navigation to the left hand side for vertical continuous manifests. There was a bit of code to hide the thumbnail panel for continuous manifests (also now using isContinuous rather than viewingHint), presumably because with this kind of material the long viewport navigation does the job thumbnail navigation was doing for non-continuous content, so it makes sense for it to be where the thumbnails normally are (and not clash with the right panel too). But the current positioning with the title and the zoom buttons looks bad; it should really be to the left of the title and the buttons (like the left panel usually is), so I'll come back to that.
Some manifests for testing: