Changes to suit Annotations from parent to iframe#4
Conversation
|
|
||
| // Check if this is manifest data that contains canvases | ||
| if (this.isManifestData(data)) { |
There was a problem hiding this comment.
Do we need this.isManifestData? if (manifestUrl) would be cheaper
| // Check if this is manifest data that contains canvases | ||
| if (this.isManifestData(data)) { | ||
| return await this.extractCanvasFromManifest(data, canvasUrl) | ||
| const pageData = await fetch(pageId) |
There was a problem hiding this comment.
Do we never want to use the AnnotationPage embedded in the Manifest's Canvas? It could save us a fetch().
|
|
||
| // Check for sequences (IIIF v2) or items with canvases (IIIF v3) | ||
| return !!(data.sequences || (data.items && Array.isArray(data.items))) | ||
| return !!(data.sequences || (data.items && Array.isArray(data.items) && data.items.some(item => item.type === "Canvas"))) |
There was a problem hiding this comment.
I don't think we need this. The check for the type is enough.
thehabes
left a comment
thehabes
left a comment
There was a problem hiding this comment.
Had a paired review at standup that resulted in some cleanup. Tested manually in the interface and this is working good. The messaging system is nice and plays nice with TPEN Interfaces.
cubap
left a comment
cubap
left a comment
There was a problem hiding this comment.
This is excellent. I have asked for some logic organization that I think will make this easier to maintain as we start adding parts and extra checks/conversions.
| if (manifestData) { | ||
| if (annotationPageData) { | ||
| return await this.extractCanvasFromManifest(manifestData, canvasData, annotationPageData) | ||
| } else { |
There was a problem hiding this comment.
this else follows a return so it isn't needed.
| } else { | ||
| return await this.extractCanvasFromManifest(manifestData, canvasData, { items: [] }) | ||
| } | ||
| } else if (canvasData) { |
| } else if (canvasData) { | ||
| if (annotationPageData) { | ||
| return await this.processDirectCanvasData(canvasData, annotationPageData) | ||
| } else if (canvasData.items && canvasData.items.length > 0 && canvasData.items[0].target) { |
| return await this.processDirectCanvasData(canvasData, annotationPageData) | ||
| } else if (canvasData.items && canvasData.items.length > 0 && canvasData.items[0].target) { | ||
| return await this.processPageData(canvasData) | ||
| } else { |
| return await this.processDirectCanvasData(canvasData, { items: [] }) | ||
| } | ||
| } else { | ||
| throw new Error("Unsupported IIIF data structure") |
There was a problem hiding this comment.
this ought to be a guard clause at the top
| this.uiManager.renderAnnotations(annotations, imgWidth, imgHeight) | ||
|
|
||
|
|
||
| let idx = null |
There was a problem hiding this comment.
this block 54-71 feel like a function that should be extracted or at least separately defined. With some returns, you could clean it up a little as well.
| } | ||
|
|
||
| if (annotationId !== null) { | ||
| document.querySelector(`.overlayBox[data-lineid="${annotationId}"]`).classList.add('clicked') |
There was a problem hiding this comment.
maybe this needs a ?. to prevent calling classlist on null if it isn't there. It is not immediately in my mind how this happens.
| document.querySelector(`.overlayBox[data-lineid="${annotationId}"]`).classList.add('clicked') | ||
| document.querySelector(`.overlayBox[data-lineid="${annotationId}"]`).setAttribute('aria-selected', 'true') | ||
| history.replaceState(null, '', `?${manifest ? `manifest=${manifest}&` : ''}canvas=${canvas}${annotationPage ? `&annotationPage=${annotationPage}` : ''}${annotation ? `&annotation=${annotation}` : ''}`) | ||
| } else { |
There was a problem hiding this comment.
with a return we can unbury these elses
| this.loadPage(canvas, manifest, annotationPage, annotation) | ||
| } else { | ||
| this.uiManager.showLoading("Waiting for canvas URL from parent window...") | ||
| this.uiManager.showLoading("Waiting for manifest URL or page URL from parent window...") |
3 participants
No description provided.