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

Refactor Figure sizing logic #982

Merged
merged 2 commits into from
Dec 18, 2019

Conversation

martinRenou
Copy link
Member

Describe your changes
Before this change, the Figure was rendered once with a default size of 640x480 before this.el was even attached to the DOM. And there was a resize when the element is actually attached to the page.

After the change, the Figure waits for this.el to be attached before doing any rendering, and it renders in the available space specified by the CSS rules (width: auto, height: 480 by default). This prevents doing an extra resize, and it prevents the figure to take a first hardcoded size.

@martinRenou martinRenou changed the title Refactor Figure sizing logic WIP: Refactor Figure sizing logic Dec 4, 2019
@maartenbreddels
Copy link
Member

This does ignore the min/max aspect ratio btw.

@martinRenou
Copy link
Member Author

Yes, we need to discuss this :)

That is the exact reason for the WIP

@maartenbreddels
Copy link
Member

I think this PR should be explored, it will cause bqplot to be more performant. However, #985 fixes the real issue mentioned in #981.

@martinRenou martinRenou changed the title WIP: Refactor Figure sizing logic Refactor Figure sizing logic Dec 16, 2019
@martinRenou
Copy link
Member Author

martinRenou commented Dec 16, 2019

@maartenbreddels @SylvainCorlay I think this is good to go! It now respects the min/max aspect ratio and it is signed off :) edit: and TypeScripted

Signed-off-by: martinRenou <martin.renou@gmail.com>
Signed-off-by: martinRenou <martin.renou@gmail.com>
@maartenbreddels
Copy link
Member

LGTM, and works.

@SylvainCorlay
Copy link
Member

Reviewed together. I approve.

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