Skip to content

Fix handling of horizontal legendgroup #3609

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

Closed
wants to merge 1 commit into from
Closed

Fix handling of horizontal legendgroup #3609

wants to merge 1 commit into from

Conversation

antoinerg
Copy link
Contributor

@antoinerg antoinerg commented Mar 6, 2019

Fixes #1913

See #1913 (comment) for details.

@antoinerg antoinerg added bug something broken status: reviewable labels Mar 6, 2019
@etpinard
Copy link
Contributor

etpinard commented Mar 7, 2019

Can you tell us how legend.tracegroupgap affects legend groups on orientation: 'h' legends now?

@antoinerg
Copy link
Contributor Author

antoinerg commented Mar 8, 2019

Can you tell us how legend.tracegroupgap affects legend groups on orientation: 'h' legends now?

@etpinard legend.tracegroupgap changes the horizontal spacing between legend items when the legend is oriented horizontally even in the absence of legend groups. If I set it to 50 in the new mock, we get:
newplot 6

I don't know if this behavior was intentional but the current PR preserves it.

@antoinerg
Copy link
Contributor Author

I think we need to introduce a new legend attribute to control the spacing between trace items independently of the spacing between grouped items. The new attribute could be named legend.tracegap and would complement the current legend.tracegroupgap.

cc @nicolaskruchten @plotly/plotly_js

@nicolaskruchten
Copy link
Contributor

I don't mind in principle, but can't we get away without it? It seems to me that horizontal legends are just like vertical ones, no?

@antoinerg
Copy link
Contributor Author

antoinerg commented Mar 12, 2019

I don't mind in principle, but can't we get away without it?

Yes we can get away without it if we're happy with solely controlling the spacing between groups of items and NOT have control over the spacing between items.

It seems to me that horizontal legends are just like vertical ones, no?

In vertical legends, grouped items are stacked vertically. In horizontal legends, they are also stacked vertically. One may argue that they should be stacked horizontally (as done in the current PR). I personally don't have a preference. If we want to keep them stacked vertically even in horizontal legends, the following community PR #3628 is more appropriate.

cc @nicolaskruchten

@nicolaskruchten
Copy link
Contributor

In horizontal legends, they are also stacked vertically

This is the current behaviour?

@antoinerg
Copy link
Contributor Author

In horizontal legends, they are also stacked vertically

This is the current behaviour?

Yes it is. This is how horizontal legends with groups are rendered on master:
newplot (10)
where items are overflowing on the right (which is essentially what we're trying to fix #1913). Also, legend.tracegroupgap which is supposed to

Sets the amount of vertical space (in px) between legend groups.

actually controls the horizontal spacing between all items if there is at least one group in the legend.

@nicolaskruchten
Copy link
Contributor

Well, if we currently stack them vertically then we can't change it, right? I think PR #3628 makes sense to me: keep vertically-aligned groups and wrap whole groups, honouring tracegroupgap. Any downsides to that @antoinerg ?

@antoinerg
Copy link
Contributor Author

antoinerg commented Mar 12, 2019

I think PR #3628 makes sense to me: keep vertically-aligned groups and wrap whole groups, honouring tracegroupgap. Any downsides to that @antoinerg ?

I agree PR #3628 is less of a breaking change. The only downside I can think of is that vertically-aligned groups will take up more space than they would if stacked horizontally.

cc @nicolaskruchten

Closing in favor of PR #3628

@antoinerg antoinerg closed this Mar 12, 2019
@antoinerg antoinerg added the regression this used to work label Mar 18, 2019
@etpinard etpinard deleted the pr-1913 branch March 18, 2019 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken regression this used to work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants