-
Notifications
You must be signed in to change notification settings - Fork 75
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
Add an annotation registry. #616
Conversation
Let renderers be selected based on the annotations that will be used. Also, there were a bunch of TODO statements asking to send a warning if items were re-registered. These have been addressed.
Current coverage is 82.80% (diff: 94.33%)@@ master #616 diff @@
==========================================
Files 85 85
Lines 7966 8019 +53
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 6590 6640 +50
- Misses 1376 1379 +3
Partials 0 0
|
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.
Most looks good to me, just has a minor suggestion.
@@ -309,6 +311,8 @@ var rectangleAnnotation = function (args) { | |||
}; | |||
inherit(rectangleAnnotation, annotation); | |||
|
|||
registerAnnotation('rectangle', rectangleAnnotation, {polygon: true}); |
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.
+1
@@ -598,6 +605,8 @@ var pointAnnotation = function (args) { | |||
}; | |||
inherit(pointAnnotation, annotation); | |||
|
|||
registerAnnotation('point', pointAnnotation, {point: true}); |
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.
My only suggestion would is that features (keys) are not visible anywhere else. Should we create a list of it somewhere at the top may be? then you can say annotation.point: true
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.
I'm not sure having another list aids much here. There is no strict reason why an annotation should need to use the same feature that any other annotation used (I don't know what a heatmap annotation would look like, but we could have one).
I think if we wanted to make this more based on definitions, then we should probably add a general
key to each base feature's capabilities map which contains the plain feature's name. Then we would import the base feature (not the renderer-specific feature) so that we could specify one of its capabilities.
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.
I just pushed a change which does this.
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.
@manthey Just as a side note, heatmap annotations look like the old-school MS Paint spray can tool. I've seen papers where this is used to allow users to generate distributions over vernacular geographic entities (e.g. downtown). There might actually be some useful applications of an annotation type like this (especially in medical imaging?)
Looks great! |
Let renderers be selected based on the annotations that will be used.
Also, there were a bunch of TODO statements asking to send a warning if items were re-registered. These have been addressed.