New: drawing dialog #364
New: drawing dialog #364
Conversation
| <button class="bp-btn-plain ${constants.CLASS_ADD_DRAWING_BTN}" | ||
| title="${__('annotation_draw_save')}"> | ||
| ${ICON_DRAW_SAVE} | ||
| Save |
There was a problem hiding this comment.
Translate these strings
| <button class="bp-btn-plain ${constants.CLASS_DELETE_DRAWING_BTN}" | ||
| title="${__('annotation_draw_delete')}"> | ||
| ${ICON_DRAW_DELETE} | ||
| Delete |
|
|
||
| if (this.deleteButtonEl) { | ||
| this.deleteButtonEl.addEventListener('click', this.deleteAnnotation); | ||
| this.deleteButtonEl.addEventListener('touchend', this.deleteAnnotation); |
There was a problem hiding this comment.
I believe click will fire in the touch chain of things. If not we we should only add the touch listeners if the browser hasTouch
| */ | ||
| setup(annotations) { | ||
| // Create outermost element container | ||
| this.element = document.createElement('div'); |
There was a problem hiding this comment.
Maybe refer to this as the wrapper or container so it's a little more specific? e.g. DrawingDialogContainerEl
There was a problem hiding this comment.
I think it's fine for the wrapper to remain as this.element since a lot of general dialog functionality is tied to this.element. We generally keep any type specific dialogs specified separately i.e. See Point annotation dialogs for example https://github.com/box/box-content-preview/blob/master/src/lib/annotations/AnnotationDialog.js#L276 vs. highlight dialogs https://github.com/box/box-content-preview/blob/master/src/lib/annotations/doc/DocHighlightDialog.js#L253
There was a problem hiding this comment.
How would this dialog be set up on mobile?
There was a problem hiding this comment.
It seems to work the same exact way on mobile (unless DOM trees are set up differently). The designs are the same for mobile and desktop.
There was a problem hiding this comment.
Sorry I meant do we need any mobile specific dialog stuff here. Assuming this will be similar to the plain highlight dialog, which has a differently styled dialogs/functionality on mobile devices
There was a problem hiding this comment.
Wait just noticed your comment about the design matching on mobile and desktop. This should be fine then.
src/lib/annotations/Annotator.js
Outdated
| * @return {void} | ||
| */ | ||
| exitAnnotationModes(mode, buttonEl) { | ||
| exitAnnotationModesExcept(mode) { |
There was a problem hiding this comment.
Maybe exitInactiveAnnotationModes?
| @@ -454,6 +454,10 @@ class DocAnnotator extends Annotator { | |||
| bindDOMListeners() { | |||
| super.bindDOMListeners(); | |||
|
|
|||
| if (!this.permissions.canAnnotate) { | |||
| } | ||
|
|
||
| this.element.removeEventListener('click', annotatorUtil.prevDefAndStopProp); | ||
| if (this.pageEl && this.pageEl.contains(this.element)) { |
There was a problem hiding this comment.
Do we expect the page element to not have the annotation dialog at some point? I think we assume that this isn't the case for other annotation types so curious if that's possible w/ draw annotations
There was a problem hiding this comment.
This is probably a correct assumption. I put this here in the case that a page was reinserted into the DOM without its child elements. (Possibly on scale/rerender or something of the sort)
| */ | ||
| setup(annotations) { | ||
| // Create outermost element container | ||
| this.element = document.createElement('div'); |
There was a problem hiding this comment.
I think it's fine for the wrapper to remain as this.element since a lot of general dialog functionality is tied to this.element. We generally keep any type specific dialogs specified separately i.e. See Point annotation dialogs for example https://github.com/box/box-content-preview/blob/master/src/lib/annotations/AnnotationDialog.js#L276 vs. highlight dialogs https://github.com/box/box-content-preview/blob/master/src/lib/annotations/doc/DocHighlightDialog.js#L253
| } | ||
|
|
||
| // Show dialog so we can get width | ||
| const clientRect = this.element.getBoundingClientRect(); |
There was a problem hiding this comment.
Can you leave a note here for me to add the annotationDialog.flipDialog implementation here :)
| * @return {void} | ||
| */ | ||
| show() { | ||
| annotatorUtil.showElement(this.element); |
There was a problem hiding this comment.
Is the mobile dialog going to be the same as the desktop one? See the show/hide methods in AnnotationDialog.js about mobile dialog specific logic
There was a problem hiding this comment.
Yep, mobile and desktop dialog will be the same.
| */ | ||
| setup(annotations) { | ||
| // Create outermost element container | ||
| this.element = document.createElement('div'); |
There was a problem hiding this comment.
How would this dialog be set up on mobile?
| * @return {void} | ||
| */ | ||
| createDialog() { | ||
| if (this.dialog) { |
There was a problem hiding this comment.
Curious as to why we would need this?
There was a problem hiding this comment.
The dialog on a drawing in progress is different than the dialog on a saved drawing. Upon saving a drawing thread, we create a new dialog that is calculated as a saved drawing. (This adds the 'user drew' label and removes the save button.)
src/lib/annotations/annotatorUtil.js
Outdated
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
| } | ||
| /** |
There was a problem hiding this comment.
Newline after the block!
| } | ||
|
|
||
| super.destroy(); | ||
| this.reset(); | ||
| this.emit('threadcleanup'); | ||
| } | ||
|
|
||
| reset() { |
Ability to save and delete drawings by selecting them and using the dialog. Also displayed while in drawing mode. Requires #362