-
-
Notifications
You must be signed in to change notification settings - Fork 607
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 setAjaxHeaders
method to Viewer and TiledImage
#2346
Conversation
- First draft, not tested at all - See openseadragon#1748
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.
This is looking good! You've done a great job of tracking down lots of details here.
Just a few thoughts...
- Allow null to clear headers (same as empty object) - Add TiledImage._updateAjaxHeaders - Add error message in case of invalid headers
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.
Coming along nicely! Thank you for adding the tests!
Looks like just a few items left :)
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.
This whole time I was confused about what the "pass falsey to override" is supposed to do... Re-reading the doc, I see it's referring to a single header in the headers object... I was thinking it was for the entire headers object! Yeah, it seems fine.
Unless you object, I think this PR is ready to be merged. |
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.
Wonderful, thank you! I agree... Let's merge it! Thank you for working through all the issues... I know it can be a pain, but it's worth making sure everything is solid.
@@ -115,7 +115,7 @@ $.ButtonGroup.prototype = { | |||
/** | |||
* Adds the given button to this button group. | |||
* | |||
* @functions | |||
* @function |
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 for fixing these, BTW!
See #1748 for background.
I haven't yet tested this at all.new TiledImage(...)
(instead of the recommendedViewer.open(...)
orViewer.addTiledImage(...)
) and setajaxHeaders
options for bothViewer
andTiledImage
. Previously,new TiledImage(options)
didn't mergeoptions.ajaxHeaders
withViewer.ajaxHeaders
, but now it does.