Skip to content
This repository has been archived by the owner. It is now read-only.

Split screenshot iframe into preselection and selection iframes #2453

Merged
merged 2 commits into from Mar 22, 2017
Merged

Conversation

@dannycoates
Copy link
Contributor

@dannycoates dannycoates commented Mar 22, 2017

closes #2162

Copy link
Contributor

@ianb ianb left a comment

Looks good! But some comments on updateElementSize and removal of anything related to scroll tracking.

this.element.style.display = "none";
}
let height = Math.max(
document.documentElement.clientHeight,
Copy link
Contributor

@ianb ianb Mar 22, 2017

Choose a reason for hiding this comment

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

updateElementSize can be simpler for this iframe. It should always be exactly window.innerHeight (and width), and doesn't have to track document changes. force also doesn't matter for this iframe.

},

hide: function () {
window.removeEventListener("scroll", this.onScroll, false);
Copy link
Contributor

@ianb ianb Mar 22, 2017

Choose a reason for hiding this comment

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

Everything related to scroll tracking can be removed. Neither iframe should require it now.

Copy link
Contributor Author

@dannycoates dannycoates Mar 22, 2017

Choose a reason for hiding this comment

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

The hoverbox remains visible during the scroll without this. I think it looks bad, but if that's not an issue I'll remove it.

Copy link
Contributor

@ianb ianb Mar 22, 2017

Choose a reason for hiding this comment

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

Ah, I didn't catch that part. That makes sense then.

currentIframe: iframePreSelection,
display: function (installHandlerOnDocument, standardOverlayCallbacks) {
return iframeSelection.display(installHandlerOnDocument)
.then(() => iframePreSelection.display(installHandlerOnDocument, standardOverlayCallbacks))
Copy link
Contributor

@ianb ianb Mar 22, 2017

Choose a reason for hiding this comment

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

;

@@ -636,8 +636,7 @@ window.uicontrol = (function () {
stateHandlers.resizing.startResize(event, "move");
} else if (! ui.Box.isControl(target)) {
mousedownPos = new Pos(event.pageX, event.pageY);
mouseupNoAutoselect = true;
setState("draggingReady");
setState("crosshairs");
Copy link
Contributor

@ianb ianb Mar 22, 2017

Choose a reason for hiding this comment

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

I suppose we should rename crosshairs as that mode hasn't displayed crosshairs for a long time. But not really related to this PR.

@ianb ianb merged commit 32b2696 into master Mar 22, 2017
3 checks passed
@ianb ianb deleted the two-iframes branch Mar 23, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants