-
Notifications
You must be signed in to change notification settings - Fork 113
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
Chore: Bundling Annotators separately in annotations.js #75
Conversation
pramodsum
commented
Apr 14, 2017
•
edited
Loading
edited
- Determines which annotator to initialize based on the viewer provided in options
- Bundles the annotators separately to reduce the size of preview.js
- NOTE: There are still annotator methods remaining in individual viewers which will be removed in future commits
Requires #81 to be merged first in order for all the unrelated unit tests to pass - MERGED |
build/webpack.config.js
Outdated
@@ -54,7 +54,8 @@ function updateConfig(conf, language, index) { | |||
const config = Object.assign(conf, { | |||
entry: { | |||
preview: [`${lib}/Preview.js`], | |||
csv: [`${lib}/viewers/text/BoxCSV.js`] | |||
csv: [`${lib}/viewers/text/BoxCSV.js`], | |||
annotations: [`${lib}/annotations/AnnotatorLoader.js`] |
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.
How about we call this BoxAnnotations.js? I realize it's a 'loader' similar to the viewer loaders, but this terminology may make it clearer it's a separate file and take us in the step of entirely decoupling annotations. I'd imagine we'd want to export something like BoxAnnotations.js from the future separate project.
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.
Ah you beat me to it!
src/lib/annotations/Annotator.js
Outdated
@@ -65,8 +65,29 @@ class Annotator extends EventEmitter { | |||
* @return {void} | |||
*/ | |||
init() { | |||
this._annotatedElement = this.getAnnotatedEl(this._container); | |||
this._annotationService = this.initAnnotationService(this.options); |
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.
We probably want to move away from this underscore = private syntax at some point since the rest of Preview doesn't use it.
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'll do it in a separate commit so it doesn't overwhelm this PR
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.
Fixed in #83
VIEWER: ['Image'], | ||
TYPE: ['point'] | ||
} | ||
]; |
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.
Good idea mirroring the viewer loaders!
/** | ||
* Determines if this loader can be used | ||
* | ||
* @param {Object} viewer - Viewer |
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.
May be good to clarify this is the viewer info object, not an instance of a viewer. Open to ideas on how we can more clearly differentiate between the two.
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.
Clarified that we are referring to the info object here. Looking into this method more... I don't think it's explicitly used to load annotators. I might be able to actually remove this method altogether
return false; | ||
} | ||
return annotator.VIEWER.indexOf(viewer) > -1; | ||
}); |
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 wonder if there's some combination of filter / find we can use instead of this check inside find
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 think this would do the same thing
return this.annotators.find(annotator => !(disabledAnnotators.includes(annotator.NAME)) && annotator.VIEWER.includes(viewer));
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.
Got it! Thanks!
@@ -44,6 +48,12 @@ describe('lib/annotations/doc/DocAnnotator', () => { | |||
stubs = {}; | |||
}); | |||
|
|||
describe('getAnnotatedEl()', () => { | |||
it('should set the annotated element as the document', () => { |
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'd do this test as it('should return the document container as the annotated element')
. You're setting annotated element explicitly in setup but that's not what getAnnotatedEl()
does
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.
Whoops! I changed the method to return the annotated element and forgot to change the test description! Good catch thanks!
@@ -28,15 +32,16 @@ describe('lib/annotations/image/ImageAnnotator', () => { | |||
annotator = null; | |||
}); | |||
|
|||
describe('getAnnotatedEl()', () => { | |||
it('should set the annotated element as the document', () => { |
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.
Same as above
src/lib/viewers/BaseViewer.js
Outdated
STATUS_SUCCESS, | ||
STATUS_VIEWABLE | ||
} from '../constants'; | ||
|
||
const JS = ['annotations.js']; |
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.
ANNOTATIONS_JS to be clear - the other viewers include actual viewer static assets in their JS const.
src/lib/viewers/BaseViewer.js
Outdated
@@ -255,6 +268,12 @@ class BaseViewer extends EventEmitter { | |||
this.addListener('togglepointannotationmode', () => { | |||
this.annotator.togglePointModeHandler(); | |||
}); | |||
|
|||
this.addListener('viewerloaded', () => { |
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.
Can you just use load
event that already exists?
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.
@tonyjin I actually wasn't sure if I should be using the 'load' event that already exists or not. If that's fine then i'll just use the load event rather than emitting a new one altogether
src/lib/viewers/BaseViewer.js
Outdated
*/ | ||
initAnnotator() { | ||
/* global AnnotatorLoader */ | ||
this.loader = new AnnotatorLoader(); |
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.
What happens if AnnotatorLoaded is not loaded? Do we still want to have annotations NOT be loaded under the cookie switch in the WebApp?
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.
Should i have it only load the AnnotatorLoader if ?showAnnotations=true is enabled?
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.
Only loading the annotations.js file if ?showAnnotaitons=true is enabled
src/lib/viewers/BaseViewer.js
Outdated
@@ -255,6 +268,12 @@ class BaseViewer extends EventEmitter { | |||
this.addListener('togglepointannotationmode', () => { | |||
this.annotator.togglePointModeHandler(); | |||
}); | |||
|
|||
this.addListener('viewerloaded', () => { |
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 didn't want to hijack the load event that already gets emitted so I emitted this event instead. Does 'viewerloaded' make sense? Or should it be renamed to something else
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 what you mean by hijack. Events can have multiple listeners so adding a listener to the load
event won't affect other listeners as long as load
is fired at time when you need it (which seems to be the case - load
means the viewer has loaded content and is ready to be interact-able).
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.
Ah sorry hijack was the wrong word. Cool I'll just use the load event then! Thanks!
build/webpack.config.js
Outdated
@@ -54,7 +54,8 @@ function updateConfig(conf, language, index) { | |||
const config = Object.assign(conf, { | |||
entry: { | |||
preview: [`${lib}/Preview.js`], | |||
csv: [`${lib}/viewers/text/BoxCSV.js`] | |||
csv: [`${lib}/viewers/text/BoxCSV.js`], | |||
annotations: [`${lib}/annotations/AnnotatorLoader.js`] |
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.
should this be named annotations or boxannotations?
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.
Let's just call it BoxAnnotations for now and rename later if needed
src/lib/viewers/BaseViewer.js
Outdated
* @protected | ||
* @return {void} | ||
*/ | ||
initAnnotator() { |
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.
maybe change this method name since we actually init the Annotator in the initAnnotations() method?
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.
Changing it to loadAnnotator
src/lib/viewers/doc/DocBaseViewer.js
Outdated
@@ -218,6 +215,8 @@ class DocBaseViewer extends BaseViewer { | |||
this.initViewer(this.pdfUrl); | |||
this.initPrint(); | |||
this.initFind(); | |||
|
|||
this.setScale(this.pdfViewer.currentScale); |
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.
was there a bug without 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.
I think I had added it as a part of the remaining refactoring of annotations but i'm not entirely sure why. Tested it and it doesn't seem to be related to a bug so removing it for now
- Moving showAnnotate() into BaseViewer - Triggering annotations load on 'viewerloaded' event