-
Notifications
You must be signed in to change notification settings - Fork 14
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
Adds args for intermediate objects #139
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks Hillel :) I agree with the rationale for not passing **kwargs
to add_annotation
.
Can we add the anno_page_id
also to the make_annotation
helper so that it can be passed through even when you don't have an Annotation object pre-created, when that function calls the now-fixed add_annotation
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks very good to me! That are only two observations that come to my mind but probably is better to address them in a separate issue:
- in the case of a Canvas you can add an Annotation to the
items
or to theannotations
list. How can you differentiate? In this case, the Annotation helper is inserting by default the annotation to the annotations list. - shall we provide the ability to set the format and the id of the body as args of the function, in case I want to show lower resolution or rotated images in the create_canvas_from_iiif?
OK, after (too many) commits, I think I address @digitaldogsbody's comment. @giacomomarchioro - I agree these are separate issues! |
I think that
This is a good idea - maybe also a separate helper targeted at the annotation body |
Adds arguments for Annotation and AnnotationPage.
I didn't add **kwargs for
AnnotationHelpers.add_annotation
because we're already passing in a pre-fab Annotation object, so since we're not creating an Annotation from scratch it makes less sense to do that (IMO). Happy to be contradicted though.Fixes #111