Fix no download available message#1705
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@jamesmisson is attempting to deploy a commit to the Universal Viewer Team on Vercel. A member of the Team first needs to authorize it. |
|
Thanks, @jamesmisson. Good catch on the missing check for high-res images being enabled. As for the rest of the code, it looks reasonable to me and I'll likely be able to approve it after I've had a chance to do a little more testing. However, I do have a refactoring suggestion that might be worth considering. Right now, we're doing all of the checks twice, which is a bit fragile since if somebody adds a new download option and forgets to update both parts of the code, things will get out of sync and a bug will be introduced. We could make this more reliable by eliminating the boolean flags and instead creating arrays of React components. In other words, rather than having code like: we could have code like: Obviously that's pseudo-code, and I can't remember all the exact syntactic details for doing this in React/JSX. I'm confident it's possible, though, and I think it may be a safer strategy. What do you think? |
|
Thanks @demiankatz , that' makes sense. I've refactored it in the last commit, is this the kind of thing you mean? |
demiankatz
left a comment
There was a problem hiding this comment.
@jamesmisson, this is a great step forward, but I think something isn't quite right somewhere. Try this:
1.) Using the default "Wunder der Vererbung" manifest, go to the configuration and disable all the individual page options like this:
{
"modules": {
"downloadDialogue": {
"options": {
"downloadCurrentViewEnabled": false,
"downloadWholeImageHighResEnabled": false,
"downloadWholeImageLowResEnabled": false
}
}
}
}
2.) After applying the configuration, you'll see that the download menu shows an "Individual Pages" heading with no options under it.
My guess is that RANGE_RENDERINGS, IMAGE_RENDERINGS and/or CANVAS_RENDERINGS are causing empty elements to be added to individualPageOptions, and that's causing the heading to display inappropriately. We might need hasRangeRenderings(), hasImageRenderings() and/or hasCanvasRenderings() methods equivalent to hasManifestRenderings() to make this work more reliably.
What do you think?
|
Thanks @demiankatz , that's done the trick. I'm still confused why the image renderings case checks for maxWidth from l. 277, but that doesn't seem to affect the fix here. |
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @jamesmisson, this looks good and is working as expected for me.
Regarding the image width check, I agree that that appears to be a bug, though maybe it's one that we should solve in a separate PR since it's not directly related to the work here.
Take a look at #1525 -- this PR was contributed a few months ago to fix a problem where disabling high-res images also disabled all renderings. The solution was to move some case statements further down, but I think they didn't get moved far enough, so some logic that should be specific to the high-res case is also being applied to the rendering cases. I think the solution is simply to move those other cases a little lower in the code. What do you think?
In any case, I'm approving this PR now. Feel free to merge it and open a follow-up to address the other issue -- or let me know if you'd prefer a different approach.
|
@jamesmisson, related to the above: I don't think the |
|
Thanks @demiankatz , I will look into that at a future opportunity and merge this for now. |
This addresses #1642 by properly displaying the 'no download available' message, and removing the headings from empty sections in the download dialogue.
The test manifest on issue 1642 was confusing things by bringing an auth issue into the mix. But this PR can be tested with other manifests without
rendering(e.g. https://iiif.io/api/cookbook/recipe/0001-mvm-image/manifest.json) and by setting the config in the configuration builder to{ "modules": { "downloadDialogue": { "options": { "downloadCurrentViewEnabled": false, "downloadWholeImageHighResEnabled": false, "downloadWholeImageLowResEnabled": false } } } }This is hopefully an incremental improvement but there are lots of outstanding issues within the download dialogue! E.g. the full range of download options listed in the downloadOptions enum aren't being checked for yet. I haven't had time to fully understand the various download options that are surfaced in a manifest, but I imagine we also want an option to download the seeAlso if it's a transcription file.