Skip to content

Fix: if annotation click is outside the doc, ignore#192

Merged
pramodsum merged 9 commits intobox:masterfrom
mingzexiao6:master
May 31, 2018
Merged

Fix: if annotation click is outside the doc, ignore#192
pramodsum merged 9 commits intobox:masterfrom
mingzexiao6:master

Conversation

@mingzexiao6
Copy link
Contributor

No description provided.

@mingzexiao6 mingzexiao6 requested a review from pramodsum May 25, 2018 18:09
@boxcla
Copy link

boxcla commented May 25, 2018

Hi @mxiao6, thanks for the pull request. Before we can merge it, we need you to sign our Contributor License Agreement. You can do so electronically here: http://opensource.box.com/cla

Once you have signed, just add a comment to this pull request saying, "CLA signed". Thanks!

@mingzexiao6
Copy link
Contributor Author

CLA signed

let [x, y] = browserCoordinates;

// If click is outside the page, ignore
if (x < 0 || x > pageWidth || y < 0 || y > pageHeight) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth moving this into a reusable util method in docUtil.js and combine the check w/ the one below

];
let [x, y] = browserCoordinates;

// Do not create annotation if event doesn't have coordinates
Copy link
Contributor

Choose a reason for hiding this comment

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

Combine comments
// Do not create annotation if event doesn't have coordinates or occurs outside the page

// Do not create annotation if event doesn't have coordinates
if (Number.isNaN(x) || Number.isNaN(y)) {
// If click is outside the page, ignore
if (!docUtil.isCoordValid(browserCoordinates, pageWidth, pageHeight)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Separate the Number.isNaN check from the docUtil.isCoordValid() check so that only the isNaN check emits an error and the boundary check does not.

@pramodsum pramodsum merged commit b74484b into box:master May 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants