Change zoom behaviour so that it maintains the top left position of the document. by KiyaDoig · Pull Request #1 · LUMIVERO/WebODF

KiyaDoig · 2017-01-20T01:15:51Z

Changes to the setZoomLevel function to maintain the scroll position in the document (meaning the line of content at the top of the page is maintained at the top when you zoom).
Tests

peitschie · 2017-01-20T01:19:53Z

+                var currentOffsetParent = offsetParent;
+                // Save the scroll ratios and related parent
+                while (currentOffsetParent) {
+                    scrollRatios.push({ container: currentOffsetParent, scrollTop: currentOffsetParent.scrollTop / zoom,


Minor, would generally have a line for each new container property when they don't all fit on the same line. And would do just a single indent. E.g.:

scrollRatios.push({ container: currentOffsetParent, scrollTop: currentOffsetParent.scrollTop / zoom, scrollLeft: currentOffsetParent.scrollLeft / zoom });

peitschie · 2017-01-20T01:20:21Z

+                scrollRatios.forEach(function(savedRatio) {
+                    currentOffsetParent.scrollTop = savedRatio.scrollTop * zoom;
+                    currentOffsetParent.scrollLeft = savedRatio.scrollLeft * zoom;
+                    currentOffsetParent = currentOffsetParent.offsetParent;


This variable shouldn't be required now, as you should be using savedRatio.container

peitschie · 2017-01-20T01:23:00Z

+    };
+
+    function setupScrollPane(width, height) {
+        t.scrollPane.style.width = width + "px";


I'd make these stateless factory methods (e.g., createScrollPane) rather than assigning to the test state like this.

@peitschie so I have done this for the others like this:

function createInnerScrollPane(width, height) { var innerScrollPane = document.createElement("div"); innerScrollPane.id = "innerScrollPane"; innerScrollPane.style.transformOrigin = "top left"; innerScrollPane.style.width = width + "px"; innerScrollPane.style.height = height + "px"; innerScrollPane.style.position = "relative"; innerScrollPane.style.overflow = "auto"; innerScrollPane.style.border = "1px solid green"; return innerScrollPane; }

Is that what you meant?
But with this one (createScrollPane) would I remove the scrollPane from the setUp method or?

I agree you ought to just inline setupScrollPane into the setUp method 😄

Haha ok, thanks.

peitschie · 2017-01-20T01:24:48Z

+        var zoom = 2;
+        t.zoomHelper.setZoomLevel(zoom);
+
+        r.shouldBe(t, "t.scrollPane.scrollTop", "100", 2);


I'd suggest a class const for 2 in this test file, as it seems on initial glance like this is somehow related to the zoom levels.

peitschie · 2017-01-20T01:27:13Z

+        t.zoomHelper.setZoomLevel(zoom);
+
+        r.shouldBe(t, "t.scrollPane.scrollTop", "12", 2);
+        r.shouldBe(t, "redSquare.getBoundingClientRect().left", "28", 2); // Zoomed out such that horizontal scroll is removed.


Should also verify the top as well.

peitschie · 2017-01-20T01:27:22Z

+
+        r.shouldBe(t, "t.scrollPane.scrollTop", "100", 2);
+        r.shouldBe(t, "t.scrollPane.scrollLeft", "160", 2);
+        r.shouldBe(t, "redSquare.getBoundingClientRect().left", "9", 2);


Might I suggest checking this in relation to the scroll BCR (bounding client rect), as you really want to assert that this is close to the top-left-most visible region of the scroll BCR in all cases, right? Applies to all other tests below

peitschie · 2017-01-20T01:29:19Z

-         * @param {!number} zoomLevel
-         * @return {undefined}
-         */
+        * Sets the zoom level of the document and maintains the top left position


The indent crept off here a bit?

KiyaDoig · 2017-01-20T03:36:31Z

Updated to address review feedback

peitschie · 2017-01-20T04:01:15Z

+    }
+
+    function setZoomLevel_Maintains_scroll_position_on_zoom_out() {
+        t.scrollPane.style.width = "100px";


Any reason you didn't put this in the setup?

Ah like have it default as that (in the setup) and the ones which need a different width set it there (in their test)?

ok :P I'll do that

KiyaDoig · 2017-01-20T04:22:47Z

Moved the width/height up to the set up

peitschie

👍 👍

Looks good to me! Will chat Monday about how this is integrated further into our build chains.

KiyaDoig · 2017-01-20T04:29:47Z

Ok, thank you 😸

peitschie self-requested a review January 20, 2017 01:18

peitschie suggested changes Jan 20, 2017

View reviewed changes

KiyaDoig force-pushed the zoom branch from b9df9bc to 36ad569 Compare January 20, 2017 03:36

peitschie reviewed Jan 20, 2017

View reviewed changes

$QSRINT\KDoig$

US8656 Change zoom behaviour to maintain top left position of document.

daf2fc9

KiyaDoig force-pushed the zoom branch from 36ad569 to daf2fc9 Compare January 20, 2017 04:21

peitschie approved these changes Jan 20, 2017

View reviewed changes

Uh oh!

Conversation

KiyaDoig commented Jan 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KiyaDoig commented Jan 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KiyaDoig Jan 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KiyaDoig commented Jan 20, 2017

Uh oh!

peitschie left a comment

Choose a reason for hiding this comment

Uh oh!

KiyaDoig commented Jan 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

KiyaDoig Jan 20, 2017 •

edited

Loading