Add canvas functions to ranges rebase by marlo-longley · Pull Request #181 · IIIF-Commons/manifesto

marlo-longley · 2026-07-21T18:56:13Z

This rebases #79

- Create getCanvases, getCanvasById, getCanvasByIndex functions for the range type

sequence to range as well - Add test for getCanvasById range

... index to each canvas in range

... ranges when getting canvases

codesandbox · 2026-07-21T18:56:19Z

Review or Edit in CodeSandbox

Open the branch in Web Editor • VS Code • Insiders

Open Preview

demiankatz

Thanks, @marlo-longley -- I didn't fully review all of the code (some of the new logic in Range.ts is pretty complex, and it will take me more time than I have available today to thoroughly study it), but please see below for a few comments and suggestions that I hope will be helpful!

demiankatz · 2026-07-21T19:30:21Z

+          (item["type"] && item["type"].toLowerCase() === "canvas") ||
+          (item["type"] && item["type"].toLowerCase() === "specificresource")


Maybe slightly more concise/efficient?

Suggested change

(item["type"] && item["type"].toLowerCase() === "canvas") ||

(item["type"] && item["type"].toLowerCase() === "specificresource")

(item["type"] && ["canvas", "specificresource"].includes(item["type"].toLowerCase()))

demiankatz · 2026-07-21T19:34:41Z

-
-          const id: string = item.id || item["@id"];
+          
+          const id: string = item.id || item["@id"] || item["source"];


Does it make sense to treat a specificResource as a canvas, and to accept id and source interchangeably like this? Or should we do different things based on type for tighter validation? (I confess that I know very little about the use case here, so I'm just asking in general terms).

Specific resource might be a canvas with a selector: https://iiif.io/api/cookbook/recipe/0025-newspaper-article-index/

{ "type": "SpecificResource", "source": { "id": "https://iiif.io/api/cookbook/recipe/0025-newspaper-article-index/canvas/p2", "type": "Canvas" }, "selector": { "type": "FragmentSelector", "value": "xywh=553,1157,470,1103" } }

But worth noting that source can be a string or a record with id/type

demiankatz · 2026-07-21T19:37:19Z

 } from "@iiif/vocabulary/dist-commonjs";

 export class Range extends ManifestResource {
+  private _canvases: Canvas[] | null = null;


It's not ideal to have _canvases and canvases as two distinct things. I think the "most correct" thing would be to rename canvases to canvasIds`, but that would be a breaking change, so we probably shouldn't do it for now. I wonder if there's something else we can do to make this less potentially confusing, though.

demiankatz · 2026-07-21T19:40:05Z

+  getTopRange(range) {
+    let parentRange = range.parentRange;
+    if (parentRange) {
+      this.getTopRange(parentRange);


Should there be a return here? Do we need to worry about malformed data causing an infinite loop?

demiankatz · 2026-07-21T19:40:47Z

+  // Alternative solution can be this
+  /*   getTopLevelCanvases(): Canvas[] {
+    if(this._canvases){
+      return this._canvases;
+    }
+
+    if (this.items.length) {
+      let canvasItems: Canvas[] = [];
+      for (let i = 0; i < this.items.length; i++) {
+        if (this.items[i].isCanvas()) {
+          canvasItems.push(<Canvas>this.items[i]);
+        }
+      }
+      return this._canvases = canvasItems;
+    }
+
+    let items = this.__jsonld.items || this.__jsonld.elements;
+
+    if (items) {
+      for (let i = 0; i < items.length; i++) {
+        if (items[i].type.includes("Canvas")) {
+          const c = items[i];
+          const canvas: Canvas = new Canvas(c, this.options);
+          canvas.index = i;
+          this.items.push(canvas);
+        }
+      }
+    } else if (this.__jsonld) {
+      for (let i = 0; i < this.__jsonld.length; i++) {
+        if (items[i].type.includes("Canvas")) {
+          const c = this.__jsonld[i];
+          const canvas: Canvas = new Canvas(c, this.options);
+          canvas.index = i;
+          this.items.push(canvas);
+        }
+      }
+    }
+
+    return this._canvases = <Canvas[]>this.items;
+  }
+ */
+


I don't think we want to put all of this commented-out code in the finished product. Do we need to weigh options and make choices?

demiankatz · 2026-07-21T19:46:23Z

    }

    const items = this.__jsonld.canvases || this.__jsonld.elements;
-


Maybe it's worth undoing this whitespace-only change to avoid a superfluous file change in the PR.

marlo-longley · 2026-07-21T20:07:08Z

@demiankatz thanks for looking!
Since I didn't write this code, just rebased it, my understanding isn't super deep, it will probably take me some time to be able to understand and make changes. I will do my best to work on this soon, but I feel no ownership/attachment if anyone else wants to make changes.

demiankatz · 2026-07-21T20:25:17Z

Sounds like we're all in the same boat, @marlo-longley! :-)

stephenwf · 2026-07-22T09:12:23Z


  private _parseTreeNode(node: TreeNode, range: Range): void {
-    node.label = <string>range.getLabel().getValue(this.options.locale);
+    node.label = < string > range.getLabel().getValue(this.options.locale);


There are a few formatting issues like this, although valid we should be consistent.

MImranAsghar added 4 commits July 21, 2026 14:52

Create canvas functions for range type

60c5a1d

- Create getCanvases, getCanvasById, getCanvasByIndex functions for the range type

Move getCanvasById function from ...

cb88d6e

sequence to range as well - Add test for getCanvasById range

Update getCanvases to assign correct ...

668d0d4

... index to each canvas in range

Add functionality to handle fragments in ...

fe1b86c

... ranges when getting canvases

marlo-longley mentioned this pull request Jul 21, 2026

Add 'get' canvas functions to ranges #79

Closed

demiankatz requested changes Jul 21, 2026

View reviewed changes

stephenwf reviewed Jul 22, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add canvas functions to ranges rebase - #181

Add canvas functions to ranges rebase#181
marlo-longley wants to merge 4 commits into
IIIF-Commons:mainfrom
marlo-longley:add-canvas-functions-to-ranges-rebase

marlo-longley commented Jul 21, 2026

Uh oh!

codesandbox Bot commented Jul 21, 2026

Uh oh!

demiankatz left a comment

Uh oh!

demiankatz Jul 21, 2026

Uh oh!

demiankatz Jul 21, 2026

Uh oh!

stephenwf Jul 22, 2026

Uh oh!

demiankatz Jul 21, 2026

Uh oh!

demiankatz Jul 21, 2026

Uh oh!

demiankatz Jul 21, 2026

Uh oh!

demiankatz Jul 21, 2026

Uh oh!

marlo-longley commented Jul 21, 2026

Uh oh!

demiankatz commented Jul 21, 2026

Uh oh!

stephenwf Jul 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		(item["type"] && item["type"].toLowerCase() === "canvas") \|\|
		(item["type"] && item["type"].toLowerCase() === "specificresource")

	(item["type"] && item["type"].toLowerCase() === "canvas") \|\|
	(item["type"] && item["type"].toLowerCase() === "specificresource")
	(item["type"] && ["canvas", "specificresource"].includes(item["type"].toLowerCase()))


		const id: string = item.id \|\| item["@id"];

		const id: string = item.id \|\| item["@id"] \|\| item["source"];

		}

		const items = this.__jsonld.canvases \|\| this.__jsonld.elements;

Uh oh!

Conversation

marlo-longley commented Jul 21, 2026

Uh oh!

codesandbox Bot commented Jul 21, 2026

Review or Edit in CodeSandbox

Uh oh!

demiankatz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marlo-longley commented Jul 21, 2026

Uh oh!

demiankatz commented Jul 21, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants