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.

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

Zoom to fit #181

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Zoom to fit #181

wants to merge 2 commits into from

Conversation

philn
Copy link

@philn philn commented Mar 24, 2020

This is based on #143, with additional fixes. Please review ;)

ystreibel and others added 2 commits March 24, 2020 12:52
There's no such `ui` anymore, this was replaced by `owner`. Also the optional
`done` argument of internalMoveBy wasn't properly checked, triggering errors
during pan navigation events.

The code style was also fixed, adding ; where appropriate.
if (!parent)
throw new Error('ui element is required to be within the scene');

function centerOn(smooth) {
Copy link
Owner

Choose a reason for hiding this comment

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

The original idea of this method was for SVG elements, that would allow clients to center on a specific element of the scene.

As the library grew this become a bit obsolete in the context of WebGL/DOM scenes (which are also supported by the library).

May I propose to keep the API, but delegate the center inference to the controller? For example, svgController API could have a method that takes the UI element and returns cx/cy, while domController does the same for the DOM nodes?

I interpret this change as a desire to zoom into the center of the actual container, right?

Choose a reason for hiding this comment

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

yes, right.


if (moveByAnimation) moveByAnimation.cancel();
var parent = owner.parentElement.parentElement;
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not following this. Why are we fetching parentElement.parentElement? Any way to avoid it?

Same applies to the use in the centerOn() method

Comment on lines +447 to +448
var wx = window.innerWidth / 2;
var wy = window.innerHeight / 2;
Copy link
Owner

Choose a reason for hiding this comment

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

This doesn't seem right for the cases when container is smaller than the window

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