Conversation
|
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. |
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @jamesmisson, and sorry it has taken me a couple of days to get back to you. See below for a couple of comments. Regarding the versioning piece, we've been pretty inconsistent about semantic versioning in the past, so a minor misstep here is not a disaster, but we should aim to be more consistent in future!
| "version": "4.3.0", | ||
| "resolved": "https://registry.npmjs.org/manifesto.js/-/manifesto.js-4.3.0.tgz", | ||
| "integrity": "sha512-UhnJ/m4KrbVUrqjlJvURakAR68y3twBXecocRiWU8MIvDJdwA1clYTtHz+in0LeYIgb6yuLZ7vStQ7ixtkkEAw==", | ||
| "version": "4.3.3", |
There was a problem hiding this comment.
The new manifesto release should probably have been 4.4.0 rather than 4.3.3 since it added a new feature rather than just fixing a bug. At very least, the package.json here should now be changed to require 4.3.3 at a minimum, because if somebody installs a lower version, the code will now break. As it is, I think it would be possible for a downstream project to include incompatible versions of manifold and manifesto without NPM catching it or complaining.
|
|
||
| public hasChoices(): boolean { | ||
| const canvas = this.getCurrentCanvas(); | ||
| return canvas.getChoices().length > 0; |
There was a problem hiding this comment.
Any reason not to use this.getChoices() here to simplify the code a little?
2 participants
This PR adds support for choices on canvases, following on from IIIF-Commons/manifesto#173.
The Helper now has:
And tests for the IIIF cookbook manifest.
I'll leave this in draft for now as it should be tested with a manifest with multiple canvases with choice.