-
Notifications
You must be signed in to change notification settings - Fork 18
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 Viewer
plugin support
#183
Add Viewer
plugin support
#183
Conversation
0432892
to
fda5703
Compare
@adamjarling @mathewjordan I updated plugins to Clover 2.7.5. |
7f3be2b
to
0c880fa
Compare
0c880fa
to
5f44adb
Compare
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.
🥇 Overall, this looks pretty good to me. I'll chat through it with @adamjarling now that I grok it well enough. If there are changes, I think they will be pretty minimal and mostly organizational.
build/build.mjs
Outdated
'annotation-helpers': { | ||
lib: { | ||
name: "CloverIIIFAnnotationHelpers", | ||
entry: "./src/lib/annotation-helpers.ts", | ||
fileName: "index", | ||
}, | ||
}, | ||
'openseadragon-helpers': { | ||
lib: { | ||
name: "CloverIIIFOpenseadragonHelpers", | ||
entry: "./src/lib/openseadragon-helpers.ts", | ||
fileName: "index", |
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 might bloat over time. I wonder if we should try to consolidate all helpers under a single export?
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 consolidated the helpers in a single export
build/root.mjs
Outdated
parseAnnotationTarget, | ||
createOpenSeadragonRect |
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.
Again, maybe we work to consolidate at some point? Perhaps if this reaches a count of three.
function renderPlugins() { | ||
return plugins | ||
.filter((plugin) => plugin.imageViewer?.menu) | ||
.map((plugin, i) => { | ||
const PluginComponent = plugin.imageViewer?.menu | ||
?.component as unknown as React.ElementType; | ||
return ( | ||
<PluginComponent | ||
key={i} | ||
{...plugin?.imageViewer?.menu?.componentProps} | ||
activeManifest={activeManifest} | ||
canvas={canvas} | ||
viewerConfigOptions={configOptions} | ||
openSeadragonViewer={openSeadragonViewer} | ||
useViewerDispatch={useViewerDispatch} | ||
useViewerState={useViewerState} | ||
></PluginComponent> | ||
); | ||
}); | ||
} |
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 think this makes sense to me!
src/context/viewer-context.tsx
Outdated
export type PluginConfig = { | ||
id: string; | ||
imageViewer?: { | ||
menu?: { | ||
component: React.ElementType; | ||
componentProps?: Record<string, unknown>; | ||
}; | ||
}; | ||
informationPanel?: { | ||
component: React.ElementType; | ||
componentProps?: Record<string, unknown>; | ||
label: InternationalString; | ||
}; | ||
}; |
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'll talk with @adamjarling about this. We'll want to make sure this naming convention is rock solid. I think imageViewer
could be canvas
to make it more generic and applicable at some point to Video/Audio and eventually 3D.
src/types/plugins.ts
Outdated
export interface PluginInformationPanel { | ||
annotations?: Annotation[]; | ||
activeManifest: string; | ||
canvas: CanvasNormalized; | ||
viewerConfigOptions: ViewerConfigOptions; | ||
openSeadragonViewer: OpenSeadragon.Viewer | null; | ||
useViewerDispatch: () => ViewerContextStore; | ||
useViewerState: () => ViewerContextStore; | ||
} |
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.
Maybe extend
PluginInformationPanel here?
export interface PluginInformationPanel extends Plugin {
annotations?: Annotation[];
}
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.
Agree here.
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 extend to PluginInformationPanel.
src/index.tsx
Outdated
parseAnnotationTarget, | ||
type AnnotationTargetExtended, | ||
createOpenSeadragonRect, | ||
type Plugin, | ||
type PluginInformationPanel, |
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 also wonder if there's cleaner way to handle these exports, or group or title them to clarify their context to future consumers of the package? Related toroot.mjs
and root.umd.js
as well.
@wykhuh @mathewjordan Should we add some documentation? |
@adamjarling I was going to add documentation after y'all looked the PR. I wanted us to figure out the api for plugins before writing the documentation. Another thing to think about: will Clover website have a page that lists the custom displays and plugins that are available for Clover? |
@mathewjordan @adamjarling You two have edit access to my forked repo, so feel free to make edits and commits to this branch. |
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.
A few notes:
- Add plugins docs in Viewer
- Consolidate helpers under one export
'helpers': {
lib: {
name: "CloverIIIFHelpers",
entry: "./src/lib/index.ts", // pile them through here
fileName: "index",
},
}
- Rename
imageViewer.menu
toimageViewer.controls
- If you'd like, add a Plugins page under Composing in the documentation nav
- Adam's note about extending types where possible
src/context/viewer-context.tsx
Outdated
export type PluginConfig = { | ||
id: string; | ||
imageViewer?: { | ||
menu?: { |
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.
menu?: { | |
controls?: { |
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 changed it to imageViewer.controls
src/types/plugins.ts
Outdated
export interface PluginInformationPanel { | ||
annotations?: Annotation[]; | ||
activeManifest: string; | ||
canvas: CanvasNormalized; | ||
viewerConfigOptions: ViewerConfigOptions; | ||
openSeadragonViewer: OpenSeadragon.Viewer | null; | ||
useViewerDispatch: () => ViewerContextStore; | ||
useViewerState: () => ViewerContextStore; | ||
} |
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.
Agree here.
@mathewjordan @adamjarling I mage the suggested changes. I also add plugin docs to the viewer page. Could you review the docs to see if it makes sense? |
Sure thing. |
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.
Looks great to me. See my one comment about consolidating helpers in the top level index.
src/index.tsx
Outdated
parseAnnotationTarget, | ||
type AnnotationTargetExtended, | ||
createOpenSeadragonRect, | ||
type Plugin, | ||
type PluginInformationPanel, |
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.
Should we also export the consolidated Helpers
here?
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.
@mathewjordan I exported a consolidated helpers.
@adamjarling @mathewjordan These are changes I made Clover in order to support plugins. The two parts that I changed for the annotation plugin was to give the plugin access to the Image Viewer Controls and Information Panel.