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

feat: Allow configuration of horizontal legend max height #7359

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

camdecoster
Copy link

@camdecoster camdecoster commented Feb 8, 2025

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.

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

@gvwilson gvwilson requested a review from emilykl February 10, 2025 14:48
@gvwilson gvwilson added feature something new P1 needed for current cycle labels Feb 10, 2025
@gvwilson
Copy link
Contributor

@emilykl please have a look - thank you

@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
Author

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

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

@@ -3396,6 +3396,13 @@
"valType": "integer"
}
},
"hmaxheight": {
Copy link
Collaborator

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

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
Author

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

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
Author

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
Author

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

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
Author

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?

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new P1 needed for current cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants