Feature: drawingSelectionUI #362
Conversation
Minh-Ng
commented
Sep 2, 2017
Minh-Ng
commented
- Select saved drawings
- Pop up dialog and position dialog in drawing mode
- Dialog implementation is in a separate PR
| @@ -454,6 +454,10 @@ class DocAnnotator extends Annotator { | |||
| bindDOMListeners() { | |||
| super.bindDOMListeners(); | |||
|
|
|||
| if (!this.permissions.canAnnotate) { | |||
There was a problem hiding this comment.
double check that this doesn't block triggering highlight annotations
There was a problem hiding this comment.
Removing these. For some reason I had thought that the upstream codebase had if (!this.canAnnotate)
| @@ -485,6 +489,12 @@ class DocAnnotator extends Annotator { | |||
| this.highlightThrottleHandle = null; | |||
| } | |||
|
|
|||
| if (!this.permissions.canAnnotate) { | |||
| @@ -135,6 +135,12 @@ annotation_highlight_toggle=Highlight text | |||
| annotation_highlight_comment=Add comment to highlighted text | |||
| # Text for which user made the highlight annotation | |||
| annotation_who_highlighted={1} highlighted | |||
| # Text for which user made the drawn annotation | |||
| annotation_who_drew={1} drew | |||
There was a problem hiding this comment.
Maybe drawn by {1}? We can double check wording with Varun
There was a problem hiding this comment.
Personally I prefer keeping this consistent w/ highlights so {1} highlighted or {1} drew
There was a problem hiding this comment.
I used '{1} drew' for consistency. This can easily be changed in the future.
src/lib/annotations/annotatorUtil.js
Outdated
| @@ -448,7 +448,8 @@ export function validateThreadParams(thread) { | |||
| export function eventToLocationHandler(locationFunction, callback) { | |||
| return (event) => { | |||
| const evt = event || window.event; | |||
| if (!evt) { | |||
| // Do nothing when the target isn't the text layer in case the text layer receives event precedence over buttons | |||
| if (!evt || (event.target && event.target.className !== 'textLayer')) { | |||
There was a problem hiding this comment.
I remember this from our conversation but is the textlayer guaranteed to be on top? Does this method only apply for documents?
There was a problem hiding this comment.
This is indeed only for documents. I have only tested this on documents with textlayers. Clearly this would cause drawing to not work on images.
There was a problem hiding this comment.
Adding a note about this for future development
| @@ -1,6 +1,8 @@ | |||
| import AnnotationDialog from '../AnnotationDialog'; | |||
|
|
|||
| class DocDrawingDialog extends AnnotationDialog { | |||
| visible = false; | |||
There was a problem hiding this comment.
JSDoc is in #364, I separated them to make it easier to read. DocDrawingDialog is simply a stub in this PR.
| this.reset(); | ||
|
|
||
| // Only make save request to server if there exist paths to save | ||
| const { undoCount } = this.pathContainer.getNumberOfItems(); | ||
| if (undoCount === 0) { | ||
| if (this.pathContainer.isUndoEmpty()) { |
There was a problem hiding this comment.
If you're reusing undo logic to determine if we should save I might rename to just isEmpty since you're already accessing the path container.
| * | ||
| * @return {void} | ||
| */ | ||
| cleanSelector() { |
There was a problem hiding this comment.
"clean" doesn't really mean much to me, maybe destroy or remove?
There was a problem hiding this comment.
changed to removeSelection
