Feature/newspaper viewer revisions (WIP) #166
Conversation
…polygon. Bounding box is not accurate still
…s an array value for annotation item body value
adamjarling
force-pushed
the
feature/newspaper-viewer--revisions
branch
from
2274b77 to
748ac8e
Compare
…uh/clover-iiif into feature/newspaper-viewer--revisions
| vttUri={annotationBody[0].id || ""} | ||
| /> | ||
| ); | ||
| default: |
There was a problem hiding this comment.
you can add another case statement for images
case annotationBodyFormat.match(/^image\//)?.input:
const imageUri =
annotationBody.find((body) => !body.id?.includes("vault://"))?.id ||
"";
return (
<AnnotationItemImage
caption={annotationBodyValue}
handleClick={handleClick}
imageUri={imageUri}
/>
);
default:
return (
<AnnotationItemPlainText
value={annotationBodyValue}
handleClick={handleClick}
/>
);
There was a problem hiding this comment.
@wykhuh Glad you mentioned this, as the renderItemBody function could be cleaned up a bit. I'm not the biggest fan of using a switch statements, but seems a decent enough use case for it here. Since the switch is on a type string, your regex matcher above could return undefined, so not entirely sure it's valid, but TypeScript is not complaining and it does logically read better not to have the image regex check tucked under the default (this seems wrong too). I'll plug your code in 😀
There was a problem hiding this comment.
That being said, I'm going to slightly re-factor this component and simplify the renderItemBody function to only do one thing (switch).
| }, | ||
| }; | ||
| } | ||
| } else if (target.includes("#t=")) { |
There was a problem hiding this comment.
@adamjarling One of the features CollectiveAccess needs is to create clips/markers/chapters for video and audio files. Sub divide a audio/video file into smaller segments; each segment has a label, start time, and end time. Can we pass in a start and end time for #t, or is it just start time? We like the way Clover handles VTT annotations in the info panel, and we would like similar functionality for the clips/markers/chapters. I can submit another pull request for this feature.
| @@ -0,0 +1,12 @@ | |||
| import React from "react"; | |||
There was a problem hiding this comment.
There is a weird bug. When there is a collection, Clover will display a dropdown menu to switch between items in the collection. If you use the menu to switch items the first time, the canvas is updated. If you use the menu to switch items 2nd, 3rd, etc times, the manifest changes but the canvas painting does not change.
http://localhost:3000/newspaper
The bug exists on the version of Clover that is running the GitHub pages.
https://samvera-labs.github.io/clover-iiif/docs/viewer/demo?iiif-content=https://iiif.io/api/cookbook/recipe/0068-newspaper/newspaper_title-collection.json
There was a problem hiding this comment.
@wykhuh @mathewjordan Spent the morning digging into this odd behavior, and getting quite familiar with vaults storage structure. Which finally pointed me to the underlying problem, or at least the cause of this bug. vault will store it's resources, ("Collection", "Annotation", "AnnotationPage", etc) keyed by the resource id. There were duplicate resource ids (URIs), for an Annotation and AnnotationPage between the two fixture files:
- public/manifest/newspaper/newspaper_issue_1.json
- public/manifest/newspaper/newspaper_issue_2.json
When switching to the 2nd manifest in public/manifest/newspaper/newspaper_collection.json, vault was overriding the value for:
http://localhost:3000/manifest/newspaper/annotation_page_painting/ap1http://localhost:3000/manifest/newspaper/annotation/p1
Updating the ids in newspaper_issue_2.json makes vault happy and it can uniquely retrieve items as expected once again.
There was a problem hiding this comment.
@adamjarling The bug also happens in the IIIF newspaper recipe https://iiif.io/api/cookbook/recipe/0068-newspaper/newspaper_title-collection.json Is the bug also caused by a duplicate id in the IIIF recipe?
There was a problem hiding this comment.
@wykhuh Hmmm, well that is interesting. I don't see any duplicate ids in that manifest, so that's not the problem, but something else is. I think we should add that recipe to the annotations test page and look into further.
|
Merged PR of this work: #170 |
Just a work in progress...