feat: Allow configuration of horizontal legend max height #7359

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

Jump to bottom

Merged

gvwilson merged 12 commits into master from cam/5106/horizontal-legend-customizable-max-height

Mar 6, 2025

Merged

feat: Allow configuration of horizontal legend max height #7359

gvwilson merged 12 commits into master from cam/5106/horizontal-legend-customizable-max-height

Mar 6, 2025

+23 −6

Conversation

camdecoster

Copy link

Contributor

@camdecoster camdecoster commented Feb 8, 2025 •

edited

Loading

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
image	image	image

Testing

Start the dev dashboard: 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

Nementon and others added 4 commits

February 8, 2025 06:49

@Nementon @camdecoster


 components / legend / add hmaxheightratio, hmaxheight attributes

60fbfe1

 - Replace hardcoded horizontal legend max height ratio of 2 with
 user options (default is still 2).

@camdecoster


 Switch to single attribute for controlling legend height

fb035f7

@camdecoster


 Update plot schema

17b86d1

@camdecoster


 Add draft log

4cbdc36

@gvwilson gvwilson requested a review from emilykl

February 10, 2025 14:48

@gvwilson gvwilson assigned camdecoster and emilykl

Feb 10, 2025

@gvwilson gvwilson added feature P1 labels

Feb 10, 2025

@gvwilson

Copy link

Contributor

gvwilson commented Feb 10, 2025

@emilykl please have a look - thank you

camdecoster added 5 commits

February 12, 2025 18:11

@camdecoster


 Merge remote-tracking branch 'origin/master' into cam/5106/horizontal...

fc26efd

...-legend-customizable-max-height

@camdecoster


 Remove debug code

c4b46a2

@camdecoster


 Update schema

1bc5212

@camdecoster


 Remove obsolete role from attribute

6387eab

@camdecoster


 Update schema

b61970f

@gvwilson gvwilson requested review from marthacryan and removed request for emilykl

February 18, 2025 19:03

@ndrezn

Copy link

Member

ndrezn commented Feb 18, 2025

For reference, which issue is this associated with?

@camdecoster

Copy link

Contributor Author

camdecoster commented Feb 18, 2025

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.

@gvwilson

Copy link

Contributor

gvwilson commented Feb 18, 2025

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

marthacryan

marthacryan reviewed

Feb 18, 2025

View reviewed changes

test/plot-schema.json Outdated

@@ -3396,6 +3396,13 @@

"valType": "integer"

}

"hmaxheight": {

Copy link

Collaborator

@marthacryan marthacryan Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Collaborator

@alexcjohnson alexcjohnson Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Contributor Author

@camdecoster camdecoster Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Collaborator

@alexcjohnson alexcjohnson Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'?

Copy link

Contributor Author

@camdecoster camdecoster Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Contributor Author

@camdecoster camdecoster Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Collaborator

@alexcjohnson alexcjohnson Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Contributor Author

@camdecoster camdecoster Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

camdecoster added 3 commits

February 21, 2025 06:10

@camdecoster


 Make maxheight apply to horizontal and vertical legends

1f55078

@camdecoster


 Update schema

2ad5ed5

@camdecoster


 Update schema

6a2abc5

@camdecoster

Copy link

Contributor Author

camdecoster commented Feb 25, 2025

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

Be on master
Start up the dashboard
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:

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

@gvwilson

Copy link

Contributor

gvwilson commented Feb 25, 2025

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

@camdecoster camdecoster requested a review from alexcjohnson

February 27, 2025 01:30

@gvwilson

Copy link

Contributor

gvwilson commented Mar 3, 2025

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:

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?
No idea what would cause the incorrect margin calculation either.
Definitely feels like a bug to me, but I can't find a report.

Unmodified mock on `master` branch

original_on_master

Modified mock on `master` branch (both changes)

mnodified_on_master

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

original_on_branch

Modified mock with both changes on this branch

modified_on_branch

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

v_only_on_branch

@camdecoster

Copy link

Contributor Author

camdecoster commented Mar 4, 2025

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.

@gvwilson

Copy link

Contributor

gvwilson commented Mar 4, 2025

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.

@camdecoster camdecoster mentioned this pull request

Mar 6, 2025

Legend size is calculated incorrectly with orientation 'v', yref 'container' #7386

Open

@camdecoster

Copy link

Contributor Author

camdecoster commented Mar 6, 2025

@gvwilson I created #7386 to track the bug.

@gvwilson

Copy link

Contributor

gvwilson commented Mar 6, 2025

Thanks @camdecoster

@gvwilson gvwilson merged commit bd35269 into master

Mar 6, 2025

1 check passed

@archmoj archmoj mentioned this pull request

Mar 29, 2025

Update map baselines and fix failing font download #7396

Merged

@camdecoster camdecoster mentioned this pull request

Jul 17, 2025

Vertical legend sizing/positioning broken #7473

Closed

Labels

feature P1

7 participants

@camdecoster @gvwilson @ndrezn @alexcjohnson @marthacryan @emilykl @Nementon

Uh oh!

feat: Allow configuration of horizontal legend max height #7359

feat: Allow configuration of horizontal legend max height #7359

Conversation

@camdecoster camdecoster commented Feb 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Demo Video or Screenshot(s):

Testing

Uh oh!

gvwilson commented Feb 10, 2025

Uh oh!

ndrezn commented Feb 18, 2025

Uh oh!

camdecoster commented Feb 18, 2025

Uh oh!

gvwilson commented Feb 18, 2025

Uh oh!

@marthacryan marthacryan Feb 18, 2025

Choose a reason for hiding this comment

Uh oh!

@alexcjohnson alexcjohnson Feb 18, 2025

Choose a reason for hiding this comment

Uh oh!

@camdecoster camdecoster Feb 19, 2025

Choose a reason for hiding this comment

Uh oh!

@alexcjohnson alexcjohnson Feb 19, 2025

Choose a reason for hiding this comment

Uh oh!

@camdecoster camdecoster Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

@camdecoster camdecoster Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

@alexcjohnson alexcjohnson Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

@camdecoster camdecoster Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

camdecoster commented Feb 25, 2025

Uh oh!

gvwilson commented Feb 25, 2025

Uh oh!

gvwilson commented Mar 3, 2025

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

Uh oh!

camdecoster commented Mar 4, 2025

Uh oh!

gvwilson commented Mar 4, 2025

Uh oh!

camdecoster commented Mar 6, 2025

Uh oh!

gvwilson commented Mar 6, 2025

Uh oh!

Uh oh!

Uh oh!

@camdecoster camdecoster commented Feb 8, 2025 •

edited

Loading

Unmodified mock on `master` branch

Modified mock on `master` branch (both changes)

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