adding support for complex choices #253
Conversation
✅ Deploy Preview for iiif-canvas-panel ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for iiif-canvas-panel-demos ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for canvas-panel-storybook canceled.
|
|
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 be99c73:
|
|
@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? |
| const choice: SingleChoice = { | ||
| type: 'single-choice', | ||
| items: nestedBodies.map((b) => ({ | ||
| id: b.id, | ||
| label: (b as any).label as any, | ||
| selected: selected.indexOf(b) !== -1, | ||
| })) as any[], | ||
| label: (ref as any).label, | ||
| }; | ||
| choiceEventChannel.emit('onChoiceChange', { choice }); |
There was a problem hiding this comment.
do you have thoughts about emitting the body.id and maybe the canvas id in what's emitted? We'd like to be able to get back to the AnnotationPage or other information and having this would be useful. It would also making deduping a lot easier.
|
I think we would need to support for parsing Since this is in the types already, code that uses the I do agree, I think we need a |
So, a pushed up a sample implementation with |
|
Yes, that's right. It's was an anticipation for a user seeing multiple choices. I guess it could be per-canvas though. |
|
hmm, ok. So, the right way forward here would be to update IIIF Helpers to return a Just trying to figure out what the best path forward is? |
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. |
@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...
As I think you've identified, we do need some means to maintain state so that when you choose one choice it doesn't flip the other one back... working on that before this is ready.the more I've thought about this, implementing state really goes against the choiceOptions (Disable / Disable all), if we do something here to maintain some choice logic, then I think we're putting the logic here while it should be in the implementing applicationgetPaintablesandextractChoicesreturning an array instead of an object (or alternately overloading the return to return an object if just one, and an array if more than one -- more backward compatible, but more complex implementation in more places)_.