-
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
New: Multi Image Annotations #73
New: Multi Image Annotations #73
Conversation
} | ||
|
||
// If no page was selected, ignore | ||
const page = Number(event.target.getAttribute('data-page-number')); |
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.
could simplify this a little
const location = super.getLocationFromEvent(event)
const page = ...;
if (location && page) {
return { page, ...location }
}
return location; #could also just return none to make it more clear what location is
* @abstract | ||
* @return {Function|null} Click handler | ||
*/ | ||
getPointModeClickHandler() {} |
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 we not do this if we're about to move annotations out of the viewers?
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.
@pramodsum Do you want me to wait to merge this in until after your separation happens? I can then pull this out, and put it into the separate repo
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.
Hmm... It'll probably take longer for the separation commit to be fully completed so might make sense to merge this in as is and i'll rebase my commit off this and adjust accordingly.
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 trying to chunk out the separation commits into smaller and easier to review bits (and into parts i'm more comfortable separating out sooner rather than later). We should be able to get one if not some of the separation commits in before this annotator so we can rebase and fix accordingly
* @return {boolean} Whether or not viewer is annotatable | ||
*/ | ||
isAnnotatable(type) { | ||
const typeIsAllowed = this.annotationTypes.some((allowed) => { |
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 about a "canAnnotateType" method? Following the model of what we do for other functionality checks like "canDownload" or "hasWebGL"
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.
@pramodsum has already solved this. See above comment about waiting or not
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 have generalized this and a few other methods in PR #75 so we might be able to get some of the refactoring in before this annotator gets merged.
…ew into New-MultiImage-Annotations
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 it might make sense to merge a portion of the annotations bundling/refactor changes ( #75 ) before merging this viewer. It will simplify some of the duplicated logic in this code review as well.
Other than that looks good! I have a few questions about zooming in the image viewer but we can discuss that in person 😄
"src/lib/viewers/box3d/model3d/Model3dAnimationClipsPullup.js", | ||
"src/lib/viewers/box3d/model3d/model3dSettingsPullup.js", | ||
"src/lib/viewers/box3d/model3d/Model3dVrControls.js", | ||
"src/lib/viewers/box3d/model3d/Model3DAnimationClipsPullup.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.
Just wanted to double check if this was intended to go w/ this commit :)
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.
It was, but I'm going to just add it into the one I'm working on for fixture renaming, and ignore it here
* @private | ||
* @return {void} | ||
*/ | ||
initAnnotations() { |
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.
Will be moved in #75
* @protected | ||
* @return {void} | ||
*/ | ||
showAnnotations() { |
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.
See #75
* @param {AnnotationService} annotationService An instance of the annotation service. | ||
*/ | ||
/* istanbul ignore next */ | ||
createAnnotator(/* fileVersionID, annotationService */) {} |
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.
After the initialization of annotator is generalized based on the viewer in #75 will we be needing this method? I think the only real difference for this between ImageAnnotator and MultiImageAnnotator is the actually initialization, right?
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 don't think we'll need it, after that
// Get image tag inside viewer | ||
const imageEl = this._annotatedElement.querySelector('img'); | ||
if (!imageEl) { | ||
// If we haven't click on an image, don't annotate. |
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 we can implement something similar for all annotators! For DocAnnotator we also check if we're inside the page element or in the dialog so we might be able to clean up some of this logic elsewhere too!
* @param {Event} event - DOM event | ||
* @return {Object|null} Location object | ||
*/ | ||
getLocationFromEvent(event) { |
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.
If this is the only thing we're creating a separate MultiImageAnnotator for, I wonder if it's possible to consolidate it into the ImageAnnotator as something that'll default to page 1 for normal images. I'm pretty sure we already include the page number in the single page image annotations location object
* @abstract | ||
* @return {Function|null} Click handler | ||
*/ | ||
getPointModeClickHandler() {} |
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 trying to chunk out the separation commits into smaller and easier to review bits (and into parts i'm more comfortable separating out sooner rather than later). We should be able to get one if not some of the separation commits in before this annotator so we can rebase and fix accordingly
@@ -30,7 +30,7 @@ class ImagePointDialog extends AnnotationDialog { | |||
const dialogWidth = dialogDimensions.width; | |||
|
|||
// Get image tag inside viewer | |||
const imageEl = this._annotatedElement.querySelector('img'); | |||
const imageEl = this._annotatedElement.querySelector(`[data-page-number="${this._location.page}"]`) || this._annotatedElement.querySelector('img'); |
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.
Add a comment that this can also be for multi-page images? Had to think a bit before I realized what this was for :)
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.
done!
@@ -312,7 +451,7 @@ class ImageBaseViewer extends BaseViewer { | |||
*/ | |||
cancelDragEvent(event) { | |||
event.preventDefault(); | |||
event.stopPropogation(); |
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.
lol
* Handles zoom | ||
* @param {string} [type] - Type of zoom in|out|reset | ||
* @private | ||
* @return {void} |
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.
Fix JSDoc ordering and spacing
* | ||
* @param {string} fileVersionID The file version id of the file to annotate. | ||
* @param {AnnotationService} annotationService An instance of the annotation service. |
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.
Dashes between param name and description
|
||
switch (type) { | ||
case 'in': | ||
newWidth = imageContainerWidth + 100; |
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.
100 moved to constant
this.emit('zoom'); | ||
|
||
// Give the browser some time to render before updating pannability | ||
setTimeout(this.updatePannability, 50); |
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.
const for timeout
this.wrapperEl.scrollLeft = (this.wrapperEl.scrollWidth - viewportWidth) / 2; | ||
|
||
if (this.annotator) { | ||
this.scaleAnnotations(this.imageEl.offsetWidth, this.imageEl.offsetHeight); |
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 we move this direct calling of annotator code into an event handler somewhere based off of the 'zoom' event + w/e data you can pass along?
if (this.controls) { | ||
this.controls.enable(); | ||
} | ||
}); |
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.
You can test these relatively easily in tests going forward by manually emitting an event after createAnnotator() is called in your tests and seeing if the imageEl has classes, controls are disabled, etc.
This should be unblocked now that #75 is merged |
@JustinHoldstock can we close this? |
Tests are almost done!