Skip to content

Conversation

@fredrikw
Copy link

This should be a fix to #4986 with an added mock to show the problem. Basically, there was a mismatch between when calculating whether a legend fits in one row and when calculating whether to add a new row, leading to a "itemGap" width difference in the row lengths. This PR addresses this by changing the calculation of row breaks.

@fredrikw
Copy link
Author

The test-image failure I believe is due to that I opted to change the calculation when breaking rows instead of when calculating if the legend is "one row only". I actually think it looks better with my changes, but I can switch if you would like me to. That way I think the test error would go away. (I'll address the missing line at the same time)

@archmoj archmoj added status: in progress bug something broken labels Sep 10, 2020
@archmoj
Copy link
Contributor

archmoj commented Sep 10, 2020

@fredrikw Could you pick the commit which adds the baseline for your new mock?
51265e5

And here is more info on how to generate new baselines: https://github.com/plotly/plotly.js/blob/master/test/image/README.md

@archmoj
Copy link
Contributor

archmoj commented Sep 10, 2020

The test-image failure I believe is due to that I opted to change the calculation when breaking rows instead of when calculating if the legend is "one row only". I actually think it looks better with my changes, but I can switch if you would like me to. That way I think the test error would go away.

I also like this new baseline:
legend_small_horizontal

Please also commit the changes to this baseline so that the test image could pass.

@archmoj
Copy link
Contributor

archmoj commented Sep 11, 2020

Thanks very much for the PR.
💃

@archmoj archmoj merged commit 94b31a8 into plotly:master Sep 11, 2020
@archmoj archmoj added this to the v1.56.0 milestone Sep 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug something broken

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants