Description

Add attribute to allow configuration of hortizontal legend maximum height using an explicit pixel value or a ratio of the layout height . This PR supersedes PR #5106, which has become stale and will be closed.

• Closes Allow configuration of horizontal legend max height (ratio || pixel) #5106

Changes

• Adds legend attribute hmaxheight
• Adds logic to allow the user to set the legend height using hmaxheight

Demo Video or Screenshot(s):

Default (50% ratio)	50 pixels	20% ratio

Testing

• Start the dev dasard: npm start
• Search for the legend_horizontal_autowrap mock
• Note how the legend displays
• Open test/image/mocks/legend_horizontal_autowrap.json
• Add the attribute hmaxheight to the legend configuration with a value of 50
• Reload the mock
• Note that the legend has been set to this value
• Update the attribute hmaxheight value to 0.2
• Note that the legend has been set to this value

@emilykl please have a look - thank you

For reference, which issue is this associated with?

For reference, which issue is this associated with?

I actually don't see one in the original PR. @gvwilson do you know of an issue in the backlog? I just searched and didn't see one.

I just searched as well and came up empty - I don't think there's an associated issue.

[marthacryan]

I wonder if there could be a more descriptive title for this config without making it crazy long? Is there a similar config that we could look at for inspiration?

[alexcjohnson]

Can we just call it maxheight and only coerce it (and document that it only applies) when the orientation is 'h'? Or actually, how hard would it be to make it also apply to vertical legends? That would be useful for example if you also have a colorbar, or if you have multiple legends, and you don't want them to overlap.

[camdecoster]

It could certainly apply to both horizontal and vertical legends. If we go that direction, should the percentage option use the full layout height or the plot height (gs.h)? Presently, horizontal legends use the full layout height, but vertical legends use the plot height as the max.

[alexcjohnson]

Interesting - in principle for both horizontal and vertical legends it should match yref, ie plot height if yref='paper' (the default). That's what you'd want for a paper-referenced legend because they and other items that they want to avoid (such as colorbars) are positioned based on the plot height. But if yref='container', its position is based on the full height so the maxheight should be as well.

I guess it gets a bit confusing for paper-referenced horizontal legends, since (exactly when the max height matters) horizontal legends push out the bottom margin, or the top margin if positioned on top, and that means the size of the legend helps determine the plot height. It may be difficult to do that calculation, since the legend may not be the only thing altering the margins - axis labels, titles, colorbars, and rangesliders can all do that too. So perhaps we need to say that maxheight as a fraction is relative to the full height except for vertical legends when yref='paper'?

[camdecoster]

I'll put something together and see what it looks like, though it sounds a bit confusing for an end user. Are there other attributes with similar caveats?

[camdecoster]

Another issue is that vertical and horizontal legends currently have different default max heights: 1 for vertical and 0.5 for horizontal. That would seem to preclude having one property.

[alexcjohnson]

Different defaults for one attribute happens a lot, you just don’t put a default in the attribute definition, you explain the conditions in the description, and you implement that logic in the supplyDefaults function.

[camdecoster]

I see. So something like the following for this line?

coerce('maxheight', isHorizontal ? 0.5 : 1);

Could someone please run through this test and tell me what you see?

• Be on master
• Start up the dasard
• Load the legend_horizontal_autowrap mock
• Note that it loads fine
• Edit the JSON for the mock to switch "orientation" to "v"
• Edit the JSON for the mock to add "yref" with a value of "container" under the "legend" key
• Reload the mock
• Note that the legend gets scrunched up into the top right corner of the plot
• Switch to this branch
• Reload the mock
• Note that an error is produced (due to a bad reaction to a negative margin value)

I have a couple of questions:

1. For the edited mock on master: is that what's it's supposed to look like?
2. For the edited mock on this branch: what might be causing the incorrect margin calculation?
3. Is this behavior the result of a bug? If so, has this been reported?

cc @alexcjohnson can you please take a look if you have a couple of minutes?

Hi @camdecoster - my apologies for taking so long to get to this. What I see is shown in the screenshots below; in answer to your questions:

1. I don't know if this is what the plot should look like with the modified JSON on master - @alexcjohnson @emilykl @ndrezn can you please tell us?
2. No idea what would cause the incorrect margin calculation either.
3. Definitely feels like a bug to me, but I can't find a report.

Unmodified mock on master branch

Modified mock on master branch (both changes)

Unmodified mock on this branch (identical to appearance on master)

Modified mock with both changes on this branch

Partially modified mock with only "orientation": "v" on this branch

Hi @camdecoster - my apologies for taking so long to get to this. What I see is shown in the screenshots below; in answer to your questions:

1. I don't know if this is what the plot should look like with the modified JSON on `master` - @alexcjohnson @emilykl @ndrezn can you please tell us?

2. No idea what would cause the incorrect margin calculation either.

3. Definitely feels like a bug to me, but I can't find a report.

Thanks for the check @gvwilson. If this is an unrelated bug (which it seems like), then I believe that there aren't any showstoppers for this PR.

It looks like an unrelated bug, so @camdecoster please file an issue that links back to this PR for reference so that we can fix it and then I'll merge this PR - thank you.

@gvwilson I created #7386 to track the bug.

Thanks @camdecoster