Repository navigation
Background panel config - #1939
Conversation
…nCenterPanel; use in component class
|
@jamesmisson is attempting to deploy a commit to the Universal Viewer Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @jamesmisson, this refactoring looks reasonable, and applying configurations seems to work as expected.
A few notes/comments:
1.) We should start building a document somewhere listing all of the backward-breaking changes in the next major release. Perhaps a MIGRATING.md file?
2.) I don't really like the way configurations are associated with modules; it feels to me like these OSD settings should live somewhere more generic, and moving the functionality from one module to another shouldn't really make any difference. But the config system we have is the config system we have, and now is not the time to change it. I just feel that it can and should be made better someday!
3.) I tested by adjusting the defaultZoomLevel setting (among others). I have no idea how defaultZoomLevel works; every value I entered caused weird results. We should probably improve the documentation for this setting -- though that's obviously an entirely separate concern, as it's not a new problem here.
|
Thanks @demiankatz , agreed on all of those points, especially number 2 - if any of these options had been needed in both components, this would have got messy. |
|
@jamesmisson, did you mean to close this instead of merging it? |
|
Nope - that's what I get for trying to use GitHub on my phone! Thanks |
Following on from the panel restructure, this splits out the
OpenSeadragonBackgroundPanelconfig from the originalOpenSeadragonCenterPanelconfig, and implements it in the new background component.To keep the established pattern of inheritance in the config, I've created a currently empty config for the general
BackgroundPaneltoo. If in the future other extensions use the center panel/background panel structure, then theBackgroundPanelbase config will come in useful.This will be a breaking change for deployments that supply an external config according to the v4 center panel config.
Sprint issue: #1875