Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes for resource and service; added thumbnail to canvas #3

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

raffazizzi
Copy link

@raffazizzi raffazizzi commented Oct 6, 2016

Hi, great library! I started using it, but I noticed a couple of issues and created fixes.

  1. Image resources should always have one resource (see API)

  2. Only one service should be specified (see API)

    So I removed the use of collections for resources and services, and added them as children instead.

  3. A thumbnail should be provided on canvases (it's recommended), so I've added an optional property.

@sdellis
Copy link
Owner

sdellis commented Oct 6, 2016

Thank you for the PR @raffazizzi ...

However, I don't see anywhere in the API that says image resources should only have one resource. And according to the Presentation API 2.1, multiple services may be specified (see services in this section). See this thread on IIIF-Discuss for clarification on cardinality of properties.

If you're looking at the examples in the spec, I agree that cardinality is confusing. In the thread I posted above, I suggested that repeatable resources should always be wrapped in an array (even if there's one). At the very least, the examples should do that for clarity around cardinality. I was overruled by the editors on both accounts. If you feel similarly, I would mention something... they may be more receptive if it wasn't just me pointing this out.

@raffazizzi
Copy link
Author

I see. In the mean time, would it make sense to parse the collection to serialize the single item if indeed it only contains one?

I agree that picking one way of representing carnality would be more consistent. Though I have seen similar patterns in the node.js world, e.g. when options parameters can be set as one string or an array of strings.

Copy link
Owner

@sdellis sdellis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand what you are saying, but that would require the Backbone/Ampersand collection module to be extended to return the model when only one exists. I like that idea, and it shouldn't be terribly hard to do.

That said, there are countless helper methods that would be cool to add to this library to make it easier to use (i.e., getting a canvas by id from a parent collection). Most front-end interactions with tabula-rasa will require the full hierarchy to be specified without adding some additional helpers. As it stands, there are three ways of accessing collection children: by id, by property ('@id' is considered a property, not id), and directly accessing the models array. See this example:

// get the first resource for the first image of a canvas with an '@id' of 'bar' 
// in the first sequence of a manifest with '_id' of 'foo' 
// which lives in a collection of an '_id' of 'c3'
// which lives in a collection of an '_id' of 'c3'

var single_resource = c.collections.get('c1').collections.get('c3').manifests.get('foo').sequences.models[0].canvases.get('bar','@id').images.models[0].resource.models[0]

I'd love to talk more about adding these helpers to make it easier. Manifesto would be a great resource for determining what's helpful.

Also, see my comments on the thumbnail property... that should be a Resource, not a string. See the manifest example in the API. If you can change this in a separate pull request, I will merge it.

@@ -1,6 +1,7 @@
import Model from 'ampersand-model'
import ImageCollection from './annotation-collection'
import AnnotationListCollection from './annotationlist-collection'
import Thumbnail from './thumbnail'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A thumbnail is a resource. This should be import Thumbnail from './resource'.

@@ -15,7 +16,8 @@ export default Model.extend({
},
label: 'string',
height: 'number',
width: 'number'
width: 'number',
thumbnail: 'string'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then this should be thumbnail: Thumbnail

@raffazizzi
Copy link
Author

raffazizzi commented Oct 6, 2016

Re: thumbnail, I agree! Though Mirador doesn't seem to like it as it expects a string... Not that I should be basing my implementation on what Mirador does, but as it's my target, I'm going to have to figure that one out (maybe with a post-processing step).

@sdellis
Copy link
Owner

sdellis commented Oct 6, 2016

Which version of Mirador are you using? It looks like the getThumbnailForCanvas method should work for both scenarios... url string or service. Thumbnail may have been a string in a previous API version, so it may be for backwards compatibility.

@raffazizzi
Copy link
Author

I'm using the 2.1 release. I see the code at that spot is the same. Will look more carefully.

@sdellis
Copy link
Owner

sdellis commented Oct 7, 2016

@raffazizzi Are you on the IIIF Slack? I'd be willing to help flesh out this library for your needs, but we should discuss more in public in the Software Developers Interest Group.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants