digirati-co-uk / iiif-canvas-panel

Investigation into component granularity
https://iiif-canvas-panel.netlify.app/
MIT License
9 stars 6 forks source link

adding support for complex choices #253

Closed abrin closed 7 months ago

abrin commented 7 months ago

@stephenwf another bug from the complexity of choices... Canvas 28 of the bayard manifest (see the choices story) has 2 different choices on the same page. But, the IIIFHelpers don't return all of those choices (there's an assumption of a single choice). This brings some of that logic internally to CanvasPanel so we can pull the choices from Vault directly...

netlify[bot] commented 7 months ago

Deploy Preview for iiif-canvas-panel ready!

Name Link
Latest commit be99c73c8859352b2ee132b7b072fef9e9062c38
Latest deploy log https://app.netlify.com/sites/iiif-canvas-panel/deploys/660da156deb17600083fadda
Deploy Preview https://deploy-preview-253--iiif-canvas-panel.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] commented 7 months ago

Deploy Preview for iiif-canvas-panel-demos ready!

Name Link
Latest commit be99c73c8859352b2ee132b7b072fef9e9062c38
Latest deploy log https://app.netlify.com/sites/iiif-canvas-panel-demos/deploys/660da1562c8ee100088a22a1
Deploy Preview https://deploy-preview-253--iiif-canvas-panel-demos.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] commented 7 months ago

Deploy Preview for canvas-panel-storybook canceled.

Name Link
Latest commit be99c73c8859352b2ee132b7b072fef9e9062c38
Latest deploy log https://app.netlify.com/sites/canvas-panel-storybook/deploys/660da1567ad28000085d7a5a
codesandbox-ci[bot] commented 7 months ago

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit be99c73c8859352b2ee132b7b072fef9e9062c38:

Sandbox Source
simple-vue Configuration
vue-3-carousel Configuration
external-stylesheet-sandbox Configuration
example-sandbox Configuration
custom-preset Configuration
react-choices-example Configuration
reacting-to-the-user Configuration
external-annotation-pages Configuration
virtual-annotation-pages Configuration
loading-annotation-pages Configuration
annotation-display-1 Configuration
annotation-display-2 Configuration
choice-example Configuration
choice-react Configuration
more-regions-1 Configuration
more-regions-2 Configuration
regions-1 Configuration
regions-2 Configuration
regions-3 Configuration
regions-4 Configuration
regions-5 Configuration
regions-6 Configuration
responsive-1 Configuration
abrin commented 7 months ago

@stephenwf https://github.com/IIIF-Commons/iiif-helpers/blob/0f582fbcf4a8899258b7a71d2216ffeb56275de4/src/painting-annotations/helper.ts#L38 part of the IIIF Commons is making an assumption that you'll only have one choice per canvas, but we have multiple in some cases. I've added a temporary fix for CanvasPanel by going to Vault directly for that info. Thoughts on the impact of changing this upstream?

stephenwf commented 7 months ago

I think we would need to support for parsing complex-choice type. https://github.com/IIIF-Commons/iiif-helpers/blob/0f582fbcf4a8899258b7a71d2216ffeb56275de4/src/painting-annotations/types.ts#L14C1-L17C2

Since this is in the types already, code that uses the single-choice should already be checking for this. So if we changed the helper to return either a single choice or a complex choice we can handle it. That way we can simply emit the whole choice object.

I do agree, I think we need a partOf or canvasId and annotationId along-side the choice. We could put it into the event emitter for now, but it feels like it should be in the choice object I think.

abrin commented 7 months ago

I think we would need to support for parsing complex-choice type. https://github.com/IIIF-Commons/iiif-helpers/blob/0f582fbcf4a8899258b7a71d2216ffeb56275de4/src/painting-annotations/types.ts#L14C1-L17C2

Since this is in the types already, code that uses the single-choice should already be checking for this. So if we changed the helper to return either a single choice or a complex choice we can handle it. That way we can simply emit the whole choice object.

I do agree, I think we need a partOf or canvasId and annotationId along-side the choice. We could put it into the event emitter for now, but it feels like it should be in the choice object I think.

So, a ComplexChoice is just saying, there is more than one choice, not that there's any grouping between the SingleChoices?

pushed up a sample implementation with partOf

stephenwf commented 7 months ago

Yes, that's right. It's was an anticipation for a user seeing multiple choices. I guess it could be per-canvas though.

abrin commented 7 months ago

hmm, ok. So, the right way forward here would be to update IIIF Helpers to return a ComplexChoice if there is more than one choice, and then to cascade the updates to CanvasPanel? Looking at the package.json, we're going from 0.9.21 of react-iiif-vault and related libraries to 1.0.6. is that a big lift?

Just trying to figure out what the best path forward is?

stephenwf commented 7 months ago

Looking at the package.json, we're going from 0.9.21 of react-iiif-vault and related libraries to 1.0.6. is that a big lift?

Yes, there is a PR with the changes required in #237 - mostly just import changes. So its not really possible at the moment to update. Forking any helpers at the moment is totally fine.