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.

Sign up for GitHub

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: Plain and Comment highlight annotations on mobile #276

Merged
merged 8 commits into from
Aug 14, 2017
Merged

New: Plain and Comment highlight annotations on mobile #276

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a60feab
Squashed commit of the following:
Aug 4, 2017
19ad58e
Chore: Moved point specific location lookup and added guard
Aug 4, 2017
fd2ff79
Chore: @SPramod PR feedback
Aug 4, 2017
f7baa6a
Chore: Using thread.reset() to remove code dup
Aug 8, 2017
238aa32
Merge branch 'master' of github.com:JustinHoldstock/box-content-previ…
Aug 8, 2017
b235c00
Chore: constant refs
Aug 8, 2017
5a10951
Chore: tests
Aug 11, 2017
c00162c
Merge branch 'master' of github.com:JustinHoldstock/box-content-previ…
Aug 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 17 additions & 13 deletions src/lib/annotations/AnnotationDialog.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,13 @@ import * as constants from './annotationConstants';
import { CLASS_ACTIVE, CLASS_HIDDEN } from '../constants';
import { ICON_CLOSE, ICON_DELETE } from '../icons/icons';

const CLASS_ANNOTATION_PLAIN_HIGHLIGHT = 'bp-plain-highlight';
const CLASS_BUTTON_DELETE_COMMENT = 'delete-comment-btn';
const CLASS_CANCEL_DELETE = 'cancel-delete-btn';
const CLASS_CANNOT_ANNOTATE = 'cannot-annotate';
const CLASS_COMMENT = 'annotation-comment';
const CLASS_COMMENTS_CONTAINER = 'annotation-comments';
const CLASS_REPLY_CONTAINER = 'reply-container';
const CLASS_REPLY_TEXTAREA = 'reply-textarea';
const CLASS_ANIMATE_DIALOG = 'bp-animate-show-dialog';
const CLASS_DELETE_CONFIRMATION = 'delete-confirmation';
const CLASS_BUTTON_DELETE_CONFIRM = 'confirm-delete-btn';

Expand All @@ -24,6 +23,7 @@ class AnnotationDialog extends EventEmitter {

/**
* The data object for constructing a dialog.
*
* @typedef {Object} AnnotationDialogData
* @property {HTMLElement} annotatedElement HTML element being annotated on
* @property {Annotation[]} annotations Annotations in dialog, can be an
Expand Down Expand Up @@ -82,8 +82,8 @@ class AnnotationDialog extends EventEmitter {
annotatorUtil.showElement(this.element);
this.element.appendChild(this.dialogEl);

if (this.highlightDialogEl && !this.hasComments) {
this.element.classList.add(CLASS_ANNOTATION_PLAIN_HIGHLIGHT);
if (this.highlightDialogEl && !this.element.querySelectorAll(`.${CLASS_COMMENT}`).length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

move this querySelectorAll() out into a const before the if statement

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

this.element.classList.add(constants.CLASS_ANNOTATION_PLAIN_HIGHLIGHT);

const headerEl = this.element.querySelector(constants.SELECTOR_MOBILE_DIALOG_HEADER);
headerEl.classList.add(CLASS_HIDDEN);
Expand All @@ -92,7 +92,7 @@ class AnnotationDialog extends EventEmitter {
const dialogCloseButtonEl = this.element.querySelector(constants.SELECTOR_DIALOG_CLOSE);
dialogCloseButtonEl.addEventListener('click', this.hideMobileDialog);

this.element.classList.add(CLASS_ANIMATE_DIALOG);
this.element.classList.add(constants.CLASS_ANIMATE_DIALOG);

this.bindDOMListeners();
}
Expand Down Expand Up @@ -147,14 +147,17 @@ class AnnotationDialog extends EventEmitter {
return;
}

this.element.classList.remove(CLASS_ANIMATE_DIALOG);
if (this.dialogEl) {
this.element.removeChild(this.dialogEl);
}
this.element.classList.remove(constants.CLASS_ANIMATE_DIALOG);

// Clear annotations from dialog
this.element.innerHTML = `
<div class="${constants.CLASS_MOBILE_DIALOG_HEADER}">
<button class="${constants.CLASS_DIALOG_CLOSE}">${ICON_CLOSE}</button>
</div>`.trim();
this.element.classList.remove(CLASS_ANNOTATION_PLAIN_HIGHLIGHT);
this.element.classList.remove(constants.CLASS_ANNOTATION_PLAIN_HIGHLIGHT);

const dialogCloseButtonEl = this.element.querySelector(constants.SELECTOR_DIALOG_CLOSE);
dialogCloseButtonEl.removeEventListener('click', this.hideMobileDialog);
Expand All @@ -163,9 +166,7 @@ class AnnotationDialog extends EventEmitter {
this.unbindDOMListeners();

// Cancel any unsaved annotations
if (!this.hasAnnotations) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Double check that this is ok to move on all other annotation types. I feel like there was a reason I added that check just forgot to add a comment explaining why

this.cancelAnnotation();
}
this.cancelAnnotation();
}

/**
Expand All @@ -174,6 +175,10 @@ class AnnotationDialog extends EventEmitter {
* @return {void}
*/
hide() {
if (this.element && this.element.classList.contains(CLASS_HIDDEN)) {
return;
}

if (this.isMobile) {
this.hideMobileDialog();
}
Expand Down Expand Up @@ -405,8 +410,7 @@ class AnnotationDialog extends EventEmitter {
// Clicking 'Cancel' button to cancel the annotation
case constants.DATA_TYPE_CANCEL:
if (this.isMobile) {
// Hide mobile dialog without destroying the thread
this.hideMobileDialog();
this.hide();
} else {
// Cancels + destroys the annotation thread
this.cancelAnnotation();
Expand Down Expand Up @@ -479,7 +483,7 @@ class AnnotationDialog extends EventEmitter {
const text = annotatorUtil.htmlEscape(annotation.text);

const annotationEl = document.createElement('div');
annotationEl.classList.add('annotation-comment');
annotationEl.classList.add(CLASS_COMMENT);
annotationEl.setAttribute('data-annotation-id', annotation.annotationID);
annotationEl.innerHTML = `
<div class="profile-image-container">${avatarHtml}</div>
Expand Down
2 changes: 1 addition & 1 deletion src/lib/annotations/AnnotationThread.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,7 @@ class AnnotationThread extends EventEmitter {
* @return {void}
*/
cancelUnsavedAnnotation() {
if (!this.isMobile && !annotatorUtil.isPending(this.state)) {
if (!annotatorUtil.isPending(this.state)) {
return;
}
this.destroy();
Expand Down
3 changes: 3 additions & 0 deletions src/lib/annotations/Annotator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
CLASS_ANNOTATION_POINT_MODE,
CLASS_MOBILE_DIALOG_HEADER,
CLASS_DIALOG_CLOSE,
ID_MOBILE_ANNOTATION_DIALOG,
SELECTOR_ANNOTATION_BUTTON_DRAW_CANCEL,
SELECTOR_ANNOTATION_BUTTON_DRAW_ENTER,
SELECTOR_ANNOTATION_BUTTON_DRAW_POST,
Expand Down Expand Up @@ -58,6 +59,7 @@ class Annotator extends EventEmitter {
this.locale = data.locale;
this.validationErrorEmitted = false;
this.isMobile = data.isMobile;
this.hasTouch = data.hasTouch;
this.previewUI = data.previewUI;
this.annotationModeHandlers = [];
}
Expand Down Expand Up @@ -123,6 +125,7 @@ class Annotator extends EventEmitter {
mobileDialogEl.classList.add(CLASS_MOBILE_ANNOTATION_DIALOG);
mobileDialogEl.classList.add(CLASS_ANNOTATION_DIALOG);
mobileDialogEl.classList.add(CLASS_HIDDEN);
mobileDialogEl.id = ID_MOBILE_ANNOTATION_DIALOG;

mobileDialogEl.innerHTML = `
<div class="${CLASS_MOBILE_DIALOG_HEADER}">
Expand Down
35 changes: 33 additions & 2 deletions src/lib/annotations/CommentBox.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ class CommentBox extends EventEmitter {
*/
parentEl;

/** Whether or not we should use touch events */
hasTouch;

/* Events that the comment box can emit. */
static CommentEvents = {
cancel: 'comment_cancel',
Expand All @@ -88,6 +91,7 @@ class CommentBox extends EventEmitter {
this.cancelText = config.cancel || this.cancelText;
this.postText = config.post || this.postText;
this.placeholderText = config.placeholder || this.placeholderText;
this.hasTouch = config.hasTouch;

// Explicit scope binding for event listeners
this.onCancel = this.onCancel.bind(this);
Expand All @@ -107,6 +111,19 @@ class CommentBox extends EventEmitter {
this.textAreaEl.focus();
}

/**
* Unfocus the text box.
*
* @return {void}
*/
blur() {
if (!this.containerEl) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just do this as

if (this.containerEl) { 
    document.activeElement.blur();
}

instead of w/ a negative check

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! Same with focus()

return;
}

document.activeElement.blur();
}

/**
* Clear out the text box.
*
Expand Down Expand Up @@ -162,6 +179,10 @@ class CommentBox extends EventEmitter {
this.containerEl = null;
this.cancelEl.removeEventListener('click', this.onCancel);
this.postEl.removeEventListener('click', this.onPost);
if (this.hasTouch) {
this.cancelEl.removeEventListener('touchstart', this.onCancel);
this.postEl.removeEventListener('touchstart', this.onPost);
}
}

//--------------------------------------------------------------------------
Expand Down Expand Up @@ -196,9 +217,12 @@ class CommentBox extends EventEmitter {
* Clear the current text in the textarea element and notify listeners.
*
* @private
* @param {Event} event Event created by input event
* @return {void}
*/
onCancel() {
onCancel(event) {
// stops touch propogating to a click event
event.preventDefault();
this.clear();
this.emit(CommentBox.CommentEvents.cancel);
}
Expand All @@ -207,9 +231,12 @@ class CommentBox extends EventEmitter {
* Notify listeners of submit event and then clear textarea element.
*
* @private
* @param {Event} event Event created by input event
* @return {void}
*/
onPost() {
onPost(event) {
// stops touch propogating to a click event
event.preventDefault();
this.emit(CommentBox.CommentEvents.post, this.textAreaEl.value);
this.clear();
}
Expand All @@ -232,6 +259,10 @@ class CommentBox extends EventEmitter {
// Add event listeners
this.cancelEl.addEventListener('click', this.onCancel);
this.postEl.addEventListener('click', this.onPost);
if (this.hasTouch) {
this.cancelEl.addEventListener('touchstart', this.onCancel);
this.postEl.addEventListener('touchstart', this.onPost);
}

return containerEl;
}
Expand Down
61 changes: 52 additions & 9 deletions src/lib/annotations/MobileAnnotator.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,11 @@
$tablet: "(min-width: 768px)";
$three-c: #ccc;
Copy link
Contributor

Choose a reason for hiding this comment

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

use $seesee from _boxuivariables.scss

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


@mixin border-top-bottom {
border-color: $three-c;
Copy link
Contributor

Choose a reason for hiding this comment

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

$seesee

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

border-style: solid;
border-width: 1px 0;
}

.bp-mobile-annotation-dialog {
background: white;
Expand All @@ -18,7 +25,7 @@ $tablet: "(min-width: 768px)";
animation: show-dialog-tablet;
animation-duration: .2s;
animation-fill-mode: forwards;
border-left: solid 1px #ccc;
border-left: solid 1px $three-c;
Copy link
Contributor

Choose a reason for hiding this comment

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

$seesee

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

width: 450px;
}
}
Expand All @@ -31,6 +38,14 @@ $tablet: "(min-width: 768px)";
}
}

.bp-create-annotation-dialog.bp-mobile-annotation-dialog {
height: auto;

.bp-annotation-highlight-dialog {
bottom: 0;
}
}

@keyframes show-dialog-small {
0% {
top: 100%;
Expand All @@ -57,32 +72,32 @@ $tablet: "(min-width: 768px)";
}

100% {
top: 1px;
top: 0;
}
}

.bp-mobile-annotation-dialog.bp-annotation-dialog,
.bp-mobile-annotation-dialog.bp-temp-annotation-dialog {
.bp-mobile-annotation-dialog.bp-annotation-dialog {
.annotation-container {
background-color: $white;
border: 0;
border-radius: 4px;
height: 100%;
overflow-x: hidden;
overflow-y: auto;
padding: 15px 15px 60px;
padding: 15px;
position: absolute;
width: 100%;
}

.bp-annotation-mobile-header {
align-items: center;
background-color: $white;
border-bottom: 1px solid #ccc;
display: flex;
height: 48px;
justify-content: space-between;
padding: 0;

@include border-top-bottom;
}

.bp-annotation-dialog-close {
Expand Down Expand Up @@ -153,9 +168,23 @@ $tablet: "(min-width: 768px)";

/* Highlight dialog */
.bp-mobile-annotation-dialog.bp-plain-highlight {
border-bottom: 1px solid #ccc;
height: 47px; // includes mobile header & highlight dialog
top: auto;

@include border-top-bottom;
}

.bp-mobile-annotation-dialog .bp-annotation-highlight-btns,
.bp-mobile-annotation-dialog .bp-create-highlight-comment,
.bp-mobile-annotation-dialog .annotation-container section[data-section="create"] {
background: white;
bottom: 0;
left: 0;
padding: 15px;
position: fixed;
width: 100%;

@include border-top-bottom;
}

.bp-mobile-annotation-dialog .bp-annotation-highlight-dialog {
Expand All @@ -169,8 +198,22 @@ $tablet: "(min-width: 768px)";
text-align: center;
z-index: 9999;

.bp-annotations-highlight-btns button {
width: 50%;
.bp-annotation-highlight-btns {
padding: 0;
button {
float: left;
width: 50%;
padding: 15px 0;

&:first-child:after {
content: "";
position: absolute;
height: 25px;
bottom: 17px;
left: 50%;
border-right: 1px solid #ccc;
Copy link
Contributor

Choose a reason for hiding this comment

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

$seesee

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

}
}
}

&.cannot-annotate {
Expand Down
11 changes: 5 additions & 6 deletions src/lib/annotations/__tests__/AnnotationDialog-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import * as annotatorUtil from '../annotatorUtil';
import * as constants from '../annotationConstants';
import { CLASS_ACTIVE, CLASS_HIDDEN } from '../../constants';

const CLASS_ANNOTATION_PLAIN_HIGHLIGHT = 'bp-plain-highlight';
const CLASS_CANCEL_DELETE = 'cancel-delete-btn';
const CLASS_CANNOT_ANNOTATE = 'cannot-annotate';
const CLASS_REPLY_TEXTAREA = 'reply-textarea';
Expand Down Expand Up @@ -166,15 +165,15 @@ describe('lib/annotations/AnnotationDialog', () => {
expect(dialog.element.classList.contains(CLASS_ANIMATE_DIALOG)).to.be.true;
});

it('should hide the mobile header if a plain highlight', () => {
it('should reset the annotation dialog to be a plain highlight if no comments are present', () => {
dialog.isMobile = true;
dialog.highlightDialogEl = {};
dialog.hasComments = false;
sandbox.stub(dialog.element, 'querySelectorAll').withArgs('.annotation-comment').returns([]);
stubs.show = sandbox.stub(annotatorUtil, 'showElement');
stubs.bind = sandbox.stub(dialog, 'bindDOMListeners');

dialog.show();
expect(dialog.element).to.have.class(CLASS_ANNOTATION_PLAIN_HIGHLIGHT);

expect(dialog.element.classList.contains(constants.CLASS_ANNOTATION_PLAIN_HIGHLIGHT)).to.be.true;
});
});

Expand All @@ -196,7 +195,7 @@ describe('lib/annotations/AnnotationDialog', () => {
dialog.hideMobileDialog();
expect(stubs.hide).to.be.called;
expect(stubs.unbind).to.be.called;
expect(stubs.cancel).to.not.be.called;
expect(stubs.cancel).to.be.called;
expect(dialog.element.classList.contains(CLASS_ANIMATE_DIALOG)).to.be.false;
});

Expand Down
2 changes: 1 addition & 1 deletion src/lib/annotations/__tests__/AnnotationThread-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -526,7 +526,7 @@ describe('lib/annotations/AnnotationThread', () => {
expect(thread.destroy).to.be.called;

// 'pending-active' state
thread.state = STATES.pending_ACTIVE;
thread.state = STATES.pending_active;
Copy link
Contributor

Choose a reason for hiding this comment

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

good catch!

thread.cancelUnsavedAnnotation();
expect(thread.destroy).to.be.called;
});
Expand Down
Loading