-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix: Update legend maxheight calculation logic #7483
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
Conversation
…end-maxheight-logic
Co-authored-by: Emily KL <[email protected]>
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.
See comments about the boolean logic and attribute description, otherwise looks good!
@camdecoster The new image test is failing, can you double check? |
src/components/legend/attributes.js
Outdated
'The reference height is the full layout height with the following exceptions: legends to the side of the plot', | ||
'or vertically oriented legends with a `yref` of `"paper". In this case, the reference height is the plot height.' |
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.
@camdecoster I think the previous version of this docstring correctly reflects what is in the code after your latest changes -- can you confirm?
'The reference height is the full layout height with the following exceptions: legends to the side of the plot', | |
'or vertically oriented legends with a `yref` of `"paper". In this case, the reference height is the plot height.' | |
'The reference height is the full layout height, except in the case of vertically oriented legends with', | |
'a `yref` of `"paper" located to the side of the plot, in which case the reference height', | |
'is the plot height.' |
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.
As we discussed, you're right. In some JS-like pseudocode:
isAboveBelow = isBelowPlotArea || isAbovePlotArea
!(isAboveBelow || !(orientation === "v" && yref === "paper")) -> !isAboveBelow && !(!(orientation === "v" && yref === "paper")) -> !isAboveBelow && (orientation === "v" && yref === "paper")
I'll switch it back the description back.
For future Cam reference, remember De Morgan's laws.
Co-authored-by: Emily KL <[email protected]>
Description
Update the legend maxheight calculation logic to account for the legend being located above/below the plot. Closes #7473.
Changes
maxheight
calculation logicDemo
Testing instructions