Skip to content
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

Merged
merged 30 commits into from
May 28, 2024

Conversation

wykhuh
Copy link
Contributor

@wykhuh wykhuh commented Mar 13, 2024

@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.

@wykhuh wykhuh force-pushed the feature/clippings-clover branch 2 times, most recently from 0432892 to fda5703 Compare April 18, 2024 19:56
@wykhuh
Copy link
Contributor Author

wykhuh commented Apr 19, 2024

@adamjarling @mathewjordan I updated plugins to Clover 2.7.5.

@wykhuh wykhuh force-pushed the feature/clippings-clover branch 2 times, most recently from 7f3be2b to 0c880fa Compare May 2, 2024 19:27
@mathewjordan mathewjordan changed the title add plugin support Add Viewer plugin support May 21, 2024
Copy link
Member

@mathewjordan mathewjordan left a 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
Comment on lines 43 to 54
'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",
Copy link
Member

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?

Copy link
Contributor Author

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
Comment on lines 17 to 18
parseAnnotationTarget,
createOpenSeadragonRect
Copy link
Member

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.

Comment on lines 90 to 109
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>
);
});
}
Copy link
Member

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!

Comment on lines 144 to 157
export type PluginConfig = {
id: string;
imageViewer?: {
menu?: {
component: React.ElementType;
componentProps?: Record<string, unknown>;
};
};
informationPanel?: {
component: React.ElementType;
componentProps?: Record<string, unknown>;
label: InternationalString;
};
};
Copy link
Member

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.

Comment on lines 16 to 24
export interface PluginInformationPanel {
annotations?: Annotation[];
activeManifest: string;
canvas: CanvasNormalized;
viewerConfigOptions: ViewerConfigOptions;
openSeadragonViewer: OpenSeadragon.Viewer | null;
useViewerDispatch: () => ViewerContextStore;
useViewerState: () => ViewerContextStore;
}
Copy link
Collaborator

@adamjarling adamjarling May 22, 2024

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[];
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree here.

Copy link
Contributor Author

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
Comment on lines 19 to 23
parseAnnotationTarget,
type AnnotationTargetExtended,
createOpenSeadragonRect,
type Plugin,
type PluginInformationPanel,
Copy link
Collaborator

@adamjarling adamjarling May 22, 2024

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.

@adamjarling
Copy link
Collaborator

@wykhuh @mathewjordan Should we add some documentation?

@wykhuh
Copy link
Contributor Author

wykhuh commented May 22, 2024

@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?

@wykhuh
Copy link
Contributor Author

wykhuh commented May 22, 2024

@mathewjordan @adamjarling You two have edit access to my forked repo, so feel free to make edits and commits to this branch.

@mathewjordan mathewjordan self-requested a review May 23, 2024 16:32
Copy link
Member

@mathewjordan mathewjordan left a 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 to imageViewer.controls
  • If you'd like, add a Plugins page under Composing in the documentation nav
  • Adam's note about extending types where possible

export type PluginConfig = {
id: string;
imageViewer?: {
menu?: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
menu?: {
controls?: {

Copy link
Contributor Author

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

Comment on lines 16 to 24
export interface PluginInformationPanel {
annotations?: Annotation[];
activeManifest: string;
canvas: CanvasNormalized;
viewerConfigOptions: ViewerConfigOptions;
openSeadragonViewer: OpenSeadragon.Viewer | null;
useViewerDispatch: () => ViewerContextStore;
useViewerState: () => ViewerContextStore;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree here.

@wykhuh
Copy link
Contributor Author

wykhuh commented May 24, 2024

@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?

@mathewjordan
Copy link
Member

@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.

@mathewjordan mathewjordan self-requested a review May 24, 2024 18:32
Copy link
Member

@mathewjordan mathewjordan left a 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
Comment on lines 19 to 23
parseAnnotationTarget,
type AnnotationTargetExtended,
createOpenSeadragonRect,
type Plugin,
type PluginInformationPanel,
Copy link
Member

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?

Copy link
Contributor Author

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.

@mathewjordan mathewjordan self-requested a review May 28, 2024 21:13
@mathewjordan mathewjordan merged commit fd6e959 into samvera-labs:main May 28, 2024
1 check passed
@mathewjordan mathewjordan deleted the feature/clippings-clover branch May 28, 2024 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants