-
-
Notifications
You must be signed in to change notification settings - Fork 595
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
Add support for IIIF Image API 3.0 beta #1764
Conversation
Thanks! I haven't looked closely at the rest, but the part that modifies 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.
Fantastic! Thank you for doing this. It all looks reasonable to me, but I'm not terribly familiar with IIIF, so I value others' input. Thank you for including the test module!
src/iiiftilesource.js
Outdated
data.version = 3; | ||
break; | ||
default: | ||
// unexpected context |
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.
It would probably be good to do a $.console.error
here. Also, perhaps we should set some sort of default version? Either that or properly error out.
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.
I added an error message.
I'm not sure I'd be able to suggest a default version. Currently version 2.x is probably the most common version, but I don't know in what situation an image service would fail to provide a correct context URI.
Any suggestion are welcome.
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.
Cool… Just having an error is probably fine then.
@eroux Thank you for taking a look! @azaroth42 Can you take a look as well? @lutzhelm What are your thoughts on the remaining bits you haven't done? Would you like to get them into this patch as well, or do you think they aren't necessary? |
|
Sorry, I mispoke in haste! 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.
The new changes look good. It seems this is ready to merge… Would you agree, @lutzhelm, @eroux, @azaroth42? If so, can you take it out of draft, @lutzhelm?
ok for me yes! |
LGTM! :) |
There we go. |
Awesome... thank you for doing this! |
Solves #1734.
Of the list in #1734 (comment) all items are implemented, except
full
region is now justmax
" - this probably refers to the size, not the region. Full / max size has already been addressed with themax
size parameter.