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

New: Added enabledTypes property. Backwards compat for disabledTypes #439

Merged

Conversation

JustinHoldstock
Copy link
Contributor

Adding enabledTypes to the annotation configuration, as well as a default enabled annotation types(DEFAULT_ENABLED), if none provided, to ANNOTATORS. This allows us to specify all of the allowed kinds of annotators, and the ones that are enabled by default (if no config provided).

IE)

// for document annotator, results in only 'point' annotations, as timestamp are not allowed
enabledTypes: ['point', 'timestamp']

Copy link
Contributor

@jeremypress jeremypress left a comment

Choose a reason for hiding this comment

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

LGTM, do you mind updating the README as well?

},
{
NAME: 'Image',
CONSTRUCTOR: ImageAnnotator,
VIEWER: ['Image', 'MultiImage'],
TYPE: [TYPES.point]
TYPE: [TYPES.point],
DEFAULT_ENABLED: [TYPES.point]
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with using DEFAULT_TYPES

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

return !viewerConfig.disabledTypes.includes(type);
});
}
const enabledTypes = viewerConfig.enabledTypes || [...modifiedAnnotator.DEFAULT_ENABLED];
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need the spread operator and then an array? Isn't modifiedAnnotator.DEFAULT_ENABLED already an array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to make a copy of the array, not modify the original

@@ -139,15 +139,16 @@ preview.show(..., {
VIEWER_NAME: {
annotations: {
enabled: Boolean, // Enables/disables if set. Respects "showAnnotations" if empty
disabledTypes: String[] // List of annotation types to disable
enabledTypes: String[] | null // List of annotation types to enable for this viewer. If empty, will respect default types for that annotator.
disabledTypes: String[] | null // DEPRECATED. List of annotation types to disable.
Copy link
Contributor

Choose a reason for hiding this comment

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

let's not add this line so people won't use disabledTypes from here on out

@JustinHoldstock JustinHoldstock merged commit 15a7712 into box:master Oct 23, 2017
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.

3 participants