Implement `layout.legend.orientation` (closes #53) by n-riesco · Pull Request #535 · plotly/plotly.js

n-riesco · 2016-05-13T21:42:08Z

Here is a PR ready for review that closes#53 (horizontal legends).

* Removed unused `i`. * Passed `legendItem` directly.

* Checked that all jasmine and image tests still pass.

* Set tspan.line's x in the callback to convertToSpans, otherwise they won't be set in the first render. * Checked all jasmine and image tests still pass.

* Ensure the position of the `tspan.line`s is set before computing the legend dimensions and expanding the margins. * The baseline image for `pseudo_html.json` needed updating because it has a multi-line legend.

* This change not only improves drawing performance, but it will also allows for `drawTexts` to be moved to `styles.js`. * Checked all jasmine and image tests still pass.

* Moved the placing of legend groups to `computeLegendDimensions` so that they are handled along with the placing of legend traces. * This change is a preliminary step to implement horizontal legends. * Checked all jasmine and image tests still pass.

* Added `legend_horizontal.json` (a mock with a horizontal legend without groups). * Added `legend_horizontal_groups.json` (a mock with a grouped horizontal legend).

jackparmer · 2016-05-13T23:38:10Z

@n-riesco Wow, awesome! Psyched about this. Do you have a few screenshots that you could drop here?

n-riesco · 2016-05-14T06:43:52Z

@jackparmer Not the prettiest shots, but here are the mocks I used for testing:

with legend groups
without legend groups

mdtusz · 2016-05-16T16:52:18Z

 }),
+ orientation: {
+ valType: 'enumerated',
+ values: ['v', 'h'],


@etpinard @cldougl @chriddyp
Do we want v/h or vertical/horizontal?

'v' / 'h' gets my vote - for parity with bar orientation.

etpinard · 2016-05-16T18:43:58Z

@n-riesco This PR is looking great. Solid work. 🍻

Not handling looooong horizontal legend like:

is ok. I don't think we should make horizontal legends horizontally scrollable ( @mdtusz agrees).

This means that we'll probably have to add some sort legend item grid attribute at some point, to allow users to make grid-like legends. But, that will be for a future PR.

jackparmer · 2016-05-16T18:46:01Z

ggplot2 users rejoice! cc @cpsievert

n-riesco · 2016-05-17T08:16:15Z

@etpinard I wouldn't mind to implement a horizontal scrollbar in another PR.
For this PR, I'll make sure that horizontal legends don't extend beyond the plotting area.

* Used `selection.call()` to invoke drawTexts, setupTraceToggle and computeTextDimentions.

* Removed legendItem from the function signatures of drawTexts, setupTraceToggle and computeTextDimensions.

etpinard · 2016-05-17T13:43:32Z

I wouldn't mind to implement a horizontal scrollbar in another PR

Actually, I don't think we should allow horizontal scrollbar ever. But yeah, not in this PR for sure.

I'll make sure that horizontal legends don't extend beyond the plotting area.

How exactly are you thinking about doing this? By truncating the legend? By making long horizontal legends span two (or more) rows? Something else?

mdtusz · 2016-05-17T14:10:29Z

Agreed on no horizontal scroll - I think it always feels very unnatural even at the best of times.

A grid layout of items when they overflow probably makes the most sense, but will be a bit of a pain to calculate with the varying text widths. If we go this route though, it will be a good opportunity to make a generalised method for "tabling" svg elements into a given width (or height too I suppose).

* Position horizontal legends on the bottom left, unless a range slider is present. * If a range slider is present, position the horizontal legend on the top left.

n-riesco · 2016-05-17T15:17:18Z

@etpinard I hadn't thought of spanning into multiple rows. I've truncated the legend width.

I can try the idea of spanning into multiple rows, but I think it won't be a small change.
Shall I leave it for a separate PR?

etpinard · 2016-05-17T15:20:54Z

I've truncated the legend width.

Yep. That's fine

Shall I leave it for a separate PR?

Absolutely. That will give us the time to think about a future grid layout.

etpinard · 2016-05-17T15:32:17Z

 coerce('orientation');
+ if(containerOut.orientation === 'h') {
+ var xaxis = layoutIn.xaxis;
+ if(xaxis && xaxis.rangeslider && xaxis.rangeslider.visible) {


Yep, that works. layout.xaxis is coerced before layout.legend. 👍

etpinard · 2016-05-17T17:45:28Z

+ var mockLayoutIn = Lib.extendDeep({}, layoutInForHorizontalLegends);
+ mockLayoutIn.xaxis.rangeslider.visible = true;
+
+ supplyLayoutDefaults(mockLayoutIn, layoutOut, []);


Great tests. Thanks

etpinard · 2016-05-17T18:26:21Z

Great PR. It doesn't solve all our legend issues, but it certainly puts us in a good place to iterate on.

Brilliant work @n-riesco 🍻

ronakvala · 2016-05-23T02:47:48Z

How can I use this feature of Legend Orientation in my code?

n-riesco · 2016-05-23T06:22:48Z

@ronakvala The two simple examples I posted above can be found here:

Nathan219 · 2018-10-24T18:47:02Z

@etpinard I hadn't thought of spanning into multiple rows. I've truncated the legend width.
I can try the idea of spanning into multiple rows, but I think it won't be a small change.
Shall I leave it for a separate PR?

What was your reasoning for truncating it? A legend isn't useful if it's cutting off things that are supposed to be listed.

Currently, I can't use vertical legends because of the negative anchor bug, and I can't use horizontal because it cuts stuff off. I don't want to group because I want people to be able to show/hide individual items.

At this point, my only choice is to implement my own legend :(.

n-riesco · 2018-10-24T19:29:12Z

@Nathan219 I'm not sure I understand. I thought that after #786 horizontal legend traces wrap to new lines.

Nathan219 · 2018-10-24T23:05:40Z

Looks like it does it when I don't use grouping. Grouped is still affected by the truncation. Looks like the anchoring issue is my main issue now, since both orientations are covering up the graph.

Thanks for the super quick response, by the way. Definitely appreciate it.

n-riesco added 11 commits May 13, 2016 21:29

Move setup of legend trace toggle into a function
ff16df1

Simplify signature of drawTexts in legend
ddc6d71
* Removed unused `i`. * Passed `legendItem` directly.

Legend: add attribute orientation
f40dbd4

Legend: add helper.isVertical(legendLayout)
92ff540

Legend: refactor computation of text dimensions
869538c
* Checked that all jasmine and image tests still pass.

Legend: set tspan.line's x in the callback
b2538e1
* Set tspan.line's x in the callback to convertToSpans, otherwise they won't be set in the first render. * Checked all jasmine and image tests still pass.

Legend: set tspan.line's x before expandMargin
9dc2609
* Ensure the position of the `tspan.line`s is set before computing the legend dimensions and expanding the margins. * The baseline image for `pseudo_html.json` needed updating because it has a multi-line legend.

Legend: compute size only after drawing texts
f68344b
* This change not only improves drawing performance, but it will also allows for `drawTexts` to be moved to `styles.js`. * Checked all jasmine and image tests still pass.

Legend: implement layout.legend.orientation
e5049c9

Legend: add mocks with horizontal legends
23e3972
* Added `legend_horizontal.json` (a mock with a horizontal legend without groups). * Added `legend_horizontal_groups.json` (a mock with a grouped horizontal legend).

n-riesco mentioned this pull request May 13, 2016
Implement layout.legend.orientation (closes #53) #534
Closed

etpinard added feature something new status: in progress labels May 16, 2016

mdtusz reviewed May 16, 2016
View reviewed changes

n-riesco added 4 commits May 17, 2016 12:02

Legend: Use D3 to invoke functions
25c2970
* Used `selection.call()` to invoke drawTexts, setupTraceToggle and computeTextDimentions.

Legend: Use Lib.setTranslate
7d502fa

Legend: replace .each() by a explicit loop
107883a

Legend: Remove legendItem from function signatures
65aa6c0
* Removed legendItem from the function signatures of drawTexts, setupTraceToggle and computeTextDimensions.

n-riesco added 2 commits May 17, 2016 15:20

Legend: set default position of horizontal legends
8c6d47a
* Position horizontal legends on the bottom left, unless a range slider is present. * If a range slider is present, position the horizontal legend on the top left.

Legend: ensure all the legends sides are visible
bc20390

etpinard reviewed May 17, 2016
View reviewed changes

n-riesco added 2 commits May 17, 2016 16:50

Legend: update baseline of legend_horizontal.json
c1632d4

Legend: test defaults for horizontal legends
313976d

etpinard reviewed May 17, 2016
View reviewed changes

etpinard added status: reviewable and removed status: in progress labels May 17, 2016

etpinard merged commit 5c91b8f into plotly:masterMay 17, 2016

n-riesco deleted the legend-orientation branch May 17, 2016 20:03

etpinard mentioned this pull request Jul 22, 2016
Legend scroll not showing on layout.legend.orientation = 'h' #766
Closed

this-username-is-taken mentioned this pull request Aug 15, 2016
Support ggplot2's theme(legend.direction = "horizontal") plotly/plotly.R#684
Open

Uh oh!

Uh oh!

Conversation

n-riesco commented May 13, 2016

Uh oh!

jackparmer commented May 13, 2016

Uh oh!

n-riesco commented May 14, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdtuszMay 16, 2016

Choose a reason for hiding this comment

Uh oh!

etpinardMay 16, 2016

Choose a reason for hiding this comment

Uh oh!

etpinard commented May 16, 2016

Uh oh!

jackparmer commented May 16, 2016

Uh oh!

n-riesco commented May 17, 2016

Uh oh!

etpinard commented May 17, 2016

Uh oh!

mdtusz commented May 17, 2016

Uh oh!

n-riesco commented May 17, 2016

Uh oh!

etpinard commented May 17, 2016

Uh oh!

etpinardMay 17, 2016

Choose a reason for hiding this comment

Uh oh!

etpinardMay 17, 2016

Choose a reason for hiding this comment

Uh oh!

etpinard commented May 17, 2016

Uh oh!

ronakvala commented May 23, 2016

Uh oh!

n-riesco commented May 23, 2016

Uh oh!

Nathan219 commented Oct 24, 2018

Uh oh!

n-riesco commented Oct 24, 2018

Uh oh!

Nathan219 commented Oct 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

n-riesco commented May 14, 2016•
edited
Loading