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 legend item wrapping for _horizontal #1229

Closed
wants to merge 3 commits into from
Closed

Fix legend item wrapping for _horizontal #1229

wants to merge 3 commits into from

Conversation

alexnb
Copy link
Contributor

@alexnb alexnb commented Dec 1, 2016

Fixes bug, which lead to not wrapping the legend if the _legendWidth was exceeded.

Fixes bug, which lead to not wrapping the legend if the _legendWidth was exceeded.
@gordonwoodhull gordonwoodhull added this to the v2.0 milestone Dec 2, 2016
@gordonwoodhull
Copy link
Contributor

Thanks @alexnb and sorry for the slow response.

I take it the bug exhibited as the row wrapping too late, i.e. an extra item gets displayed on each row and pushes out of bounds?

I'm going to try adding a failing test that will get fixed by this.

gordonwoodhull added a commit that referenced this pull request Jan 3, 2017
for #1229

and this is what happens when you punt on testing the expected behavior
and just test that something happened.

(i blame myself in bb549a1, where i improved the test but still didn't
look at where it was wrapping)
@gordonwoodhull
Copy link
Contributor

I was able to confirm the bug in a test, and merged your fix for v2.0. Thanks @alexnb!

It was a sophisticated kind of off-by-one error, and in reviewing this I noticed that we deliberately didn't test this feature well enough when merging it. Partly I've just gotten better at writing tests (or more patient). Partly it's messy to test anything involving text because the sizes can come out wildly different (especially on PhantomJS).

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.

2 participants