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

Upgrade Plotly #4752

Merged
merged 5 commits into from
Apr 6, 2020
Merged

Upgrade Plotly #4752

merged 5 commits into from
Apr 6, 2020

Conversation

kravets-levko
Copy link
Collaborator

@kravets-levko kravets-levko commented Mar 23, 2020

What type of PR is this? (check all applicable)

  • Other

Description

Upgrade Plotly to latest version to use new features (like offsetgroup needed for #4150) also for smoother upgrades in future.

I tested Chart visualization a lot after upgrade and didn't notice anything critical. Seems Plotly now computes auto-margins in a different way - I adjusted margins on our side to keep it as close as possible to previous version. Also, it changed legend styles a bit (see screenshots before/after) - unfortunately, I didn't find a good fix:

image
image

Related Tickets & Documents

#4150

@kravets-levko kravets-levko force-pushed the upgrade-plotly branch 2 times, most recently from df07124 to b6cbdd1 Compare March 30, 2020 16:36
@kravets-levko kravets-levko force-pushed the upgrade-plotly branch 2 times, most recently from 5fff5e0 to 7a89541 Compare March 30, 2020 18:05
Copy link
Member

@arikfr arikfr left a comment

Choose a reason for hiding this comment

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

The difference in legend style is its width/where the scroll is?

@@ -118,10 +118,10 @@ function prepareBoxLayout(layout, options, data) {

export default function prepareLayout(element, options, data) {
const layout = {
margin: { l: 10, r: 10, b: 10, t: 25, pad: 4 },
margin: { l: 10, r: 10, b: 5, t: 20, pad: 4 },
Copy link
Member

Choose a reason for hiding this comment

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

Without this change there are just bigger margins?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, and for me it looked too large; left/right paddings also changed a little bit, but in all cases it depends on axis labels (seems Plotly changed an algorithm that measures labels and now it preserves few more pixels for labels). Also seems that extra space also depends on chart size, therefore I was unable to fit margins perfectly, just as close as possible

width: Math.floor(element.offsetWidth),
height: Math.floor(element.offsetHeight),
autosize: true,
autosize: false,
Copy link
Member

Choose a reason for hiding this comment

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

What do we lose by disabling autosize?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Built-in autosize feature is based on window.onresize, and since we have implemented this feature on our own (via resizeObserver service which is even more powerful) - we don't really need it. So turned it off just to save some resources.

@kravets-levko
Copy link
Collaborator Author

The difference in legend style is its width/where the scroll is?

Yes, and there is no any setting to control it. I tried to fix it with CSS, but it has some internal state and after any user interaction it returns to its default look.

@kravets-levko kravets-levko added the dependencies Pull requests that update a dependency file label Apr 1, 2020
@arikfr arikfr merged commit 51b5732 into master Apr 6, 2020
@arikfr arikfr deleted the upgrade-plotly branch April 6, 2020 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file Frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants