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

Jump to bottom

Fix: Point image annotations popover should be on annotated element #305

Merged
merged 4 commits into from
Nov 30, 2018
Merged

Fix: Point image annotations popover should be on annotated element #305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3e37d2b
Fix: Popover for Image Points should be annotated element
Nov 30, 2018
24430a8
Fix: Position of image flipped popover
Nov 30, 2018
b3ffab9
Chore: adding unit test
Nov 30, 2018
e810eab
Merge branch 'master' into point-image-annotations
Nov 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 13 additions & 3 deletions src/image/ImagePointThread.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,17 @@ import {
} from '../constants';

class ImagePointThread extends AnnotationThread {
/**
* Gets the popover parent for image point threads. The popover parent
* should be not the image element but rather the annotatedElement
*
* @override
* @return {HTMLElement} The correct parent based on mobile view or not
*/
getPopoverParent() {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like DocPointThread uses the base method defined in Annotation thread whereas this one needs separate logic. How do you feel about moving the base method to DocPointThread and enforcing that each annotation type implement this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer to keep it in AnnotationThread since this logic seems to work for the majority of the annotation threads (point, drawing, and highlight). Only ImagePointThread needs to override this base functionality. Thoughts, @pramodsum?

Copy link
Contributor

Choose a reason for hiding this comment

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

fair enough!

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally it would be re-usable. I think that ambition may have actually caused me to accidentally wipe this out in the first place :(

ConradJChan marked this conversation as resolved.
Show resolved Hide resolved
return shouldDisplayMobileUI(this.container) ? this.container : this.annotatedElement;
}

/** @inheritdoc */
show() {
const [browserX, browserY] = getBrowserCoordinatesFromLocation(this.location, this.annotatedElement);
Expand Down Expand Up @@ -50,9 +61,8 @@ class ImagePointThread extends AnnotationThread {
const flippedPopoverOffset = isUpperHalf
? 0
: popoverEl.getBoundingClientRect().height +
POINT_ANNOTATION_ICON_HEIGHT +
ANNOTATION_POPOVER_CARET_HEIGHT +
POINT_ANNOTATION_ICON_DOT_HEIGHT;
POINT_ANNOTATION_ICON_DOT_HEIGHT * 2 +
ANNOTATION_POPOVER_CARET_HEIGHT;

const dialogTopY =
this.element.offsetTop +
Expand Down
28 changes: 27 additions & 1 deletion src/image/__tests__/ImagePointThread-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,24 @@ const html = `<div class="annotated-element" data-page-number="1">

describe('image/ImagePointThread', () => {
let rootElement;
let container;
let annotatedElement;

beforeEach(() => {
rootElement = document.createElement('div');
rootElement.innerHTML = html;
document.body.appendChild(rootElement);

container = document.createElement('div');
container.classList.add('container');

annotatedElement = document.querySelector(SELECTOR_ANNOTATED_ELEMENT);

thread = new ImagePointThread({
annotatedElement: document.querySelector(SELECTOR_ANNOTATED_ELEMENT),
annotatedElement,
annotations: [],
api: {},
container,
fileVersionId: 1,
location: {},
threadID: 2,
Expand Down Expand Up @@ -73,4 +81,22 @@ describe('image/ImagePointThread', () => {
expect(thread.renderAnnotationPopover).not.toBeCalled();
});
});

describe('getPopoverParent()', () => {
beforeEach(() => {
util.shouldDisplayMobileUI = jest.fn();
});

it('should return the container if mobile', () => {
util.shouldDisplayMobileUI.mockReturnValueOnce(true);

expect(thread.getPopoverParent()).toBe(container);
});

it('should return the annotatedEleemnt if not mobile', () => {
util.shouldDisplayMobileUI.mockReturnValueOnce(false);

expect(thread.getPopoverParent()).toBe(annotatedElement);
});
});
});