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

Fix disappearing legend entries #5139

Merged
merged 4 commits into from
Sep 11, 2020
Merged

Fix disappearing legend entries #5139

merged 4 commits into from
Sep 11, 2020

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