Skip to content

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/components/legend/attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,9 @@ module.exports = {
'Sets the max height (in px) of the legend, or max height ratio (reference height * ratio) if less than one.',
'Default value is: 0.5 for horizontal legends; 1 for vertical legends. The minimum allowed height is 30px.',
'For a ratio of 0.5, the legend will take up to 50% of the reference height before displaying a scrollbar.',
'The reference height is the full layout height except for vertically oriented legends with',
'a `yref` of `"paper"`, where the reference height is the plot height.'
'The reference height is the full layout height with the following exception: vertically oriented legends with',
'a `yref` of `"paper"`, unless the legend is located above/below the plot. In this case, the reference height',
'is the plot height.'
].join(' ')
},
borderwidth: {
Expand Down Expand Up @@ -243,7 +244,7 @@ module.exports = {
values: ['auto', 'top', 'middle', 'bottom'],
editType: 'legend',
description: [
'Sets the legend\'s vertical position anchor',
'Sets the legend\'s vertical position anchor.',
'This anchor binds the `y` position to the *top*, *middle*',
'or *bottom* of the legend.',
'Value *auto* anchors legends at their bottom for `y` values less than or equal to 1/3,',
Expand Down
2 changes: 1 addition & 1 deletion src/components/legend/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ function groupDefaults(legendId, layoutIn, layoutOut, fullData) {

coerce('xanchor', defaultXAnchor);
coerce('yanchor', defaultYAnchor);
coerce('maxheight', isHorizontal ? 0.5 : 1);
coerce('maxheight');
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the attribute description: "Default value is: 0.5 for horizontal legends; 1 for vertical legends."
So it looks like we should keep it as

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

coerce('valign');
Lib.noneOrAll(containerIn, containerOut, ['x', 'y']);

Expand Down
8 changes: 6 additions & 2 deletions src/components/legend/draw.js
Original file line number Diff line number Diff line change
Expand Up @@ -769,8 +769,12 @@ function computeLegendDimensions(gd, groups, traces, legendObj) {
var traceGroupGap = legendObj.tracegroupgap;
var legendGroupWidths = {};

var { maxheight, orientation, yref } = legendObj;
var heightToBeScaled = orientation === "v" && yref === "paper" ? gs.h : fullLayout.height;
const { orientation, yref } = legendObj;
let { maxheight } = legendObj;
const useFullLayoutHeight = isBelowPlotArea || isAbovePlotArea && !(orientation === "v" && yref === "paper")
// Set default maxheight here since it depends on values passed in by user
maxheight ||= useFullLayoutHeight ? 0.5 : 1;
const heightToBeScaled = useFullLayoutHeight ? fullLayout.height : gs.h;
legendObj._maxHeight = Math.max(maxheight > 1 ? maxheight : maxheight * heightToBeScaled, 30);

var toggleRectWidth = 0;
Expand Down
2 changes: 1 addition & 1 deletion tasks/find_locale_strings.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ function findLocaleStrings() {
var code = fs.readFileSync(file, 'utf-8');
var filePartialPath = file.substr(constants.pathToSrc.length);

falafel(code, {locations: true}, function(node) {
falafel(code, { ecmaVersion: 12, locations: true }, function(node) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@camdecoster Would you please open a separate PR for these adjustments to falafel arguments? That way we could merge that one quickly and benefit from it in other new PRs.

if(node.type === 'CallExpression' &&
(node.callee.name === '_' || node.callee.source() === 'Lib._')
) {
Expand Down
4 changes: 2 additions & 2 deletions tasks/test_syntax.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ function assertJasmineSuites() {
var code = fs.readFileSync(file, 'utf-8');
var bn = path.basename(file);

falafel(code, {locations: true}, function(node) {
falafel(code, { ecmaVersion: 12, locations: true }, function(node) {
var lineInfo = '[line ' + node.loc.start.line + '] :';

if(node.type === 'Identifier' && BLACK_LIST.indexOf(node.name) !== -1) {
Expand Down Expand Up @@ -108,7 +108,7 @@ function assertSrcContents() {

// parse through code string while keeping track of comments
var comments = [];
falafel(code, {onComment: comments, locations: true}, function(node) {
falafel(code, { ecmaVersion: 12, locations: true, onComment: comments }, function(node) {
// look for .classList
if(node.type === 'MemberExpression') {
var source = node.source();
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading