-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Merged
gvwilson
merged 12 commits into
master
from
cam/5106/horizontal-legend-customizable-max-height
Mar 6, 2025
Merged
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
60fbfe1
components / legend / add `hmaxheightratio`, `hmaxheight` attributes
Nementon fb035f7
Switch to single attribute for controlling legend height
camdecoster 17b86d1
Update plot schema
camdecoster 4cbdc36
Add draft log
camdecoster fc26efd
Merge remote-tracking branch 'origin/master' into cam/5106/horizontal…
camdecoster c4b46a2
Remove debug code
camdecoster 1bc5212
Update schema
camdecoster 6387eab
Remove obsolete role from attribute
camdecoster b61970f
Update schema
camdecoster 1f55078
Make maxheight apply to horizontal and vertical legends
camdecoster 2ad5ed5
Update schema
camdecoster 6a2abc5
Update schema
camdecoster File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
- Allow configuration of horizontal legend max height [[#7359](https://github.com/plotly/plotly.js/pull/7359)] |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.There was a problem hiding this comment.
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 ifyref='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 ifyref='container'
, its position is based on the full height so themaxheight
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 whenyref='paper'
?There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?