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.

Sign up for GitHub

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

Expose minreducedwidth and minreducedheight to API #6307

Merged
merged 14 commits into from
Sep 7, 2022
Merged

Expose minreducedwidth and minreducedheight to API #6307

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
aeaba5c
Expose variables
hannahker Aug 31, 2022
f826ef7
Add variable description
hannahker Sep 1, 2022
bb2ca73
Add mocks for image tests
hannahker Sep 2, 2022
1c7cc49
Remove console.log
hannahker Sep 2, 2022
61807ba
Reformat axis titles in mock
hannahker Sep 2, 2022
d692c7e
Update minimum and add units to description
hannahker Sep 2, 2022
7fc8514
Add jasmine test
hannahker Sep 2, 2022
c53e084
Update baseline images and layoutDefaults jasmine test
hannahker Sep 2, 2022
2fc928d
Fix trailing whitespace
hannahker Sep 2, 2022
0effd0f
Update plot schema
hannahker Sep 2, 2022
6a43af4
Make test more flexible for CircleCI
hannahker Sep 2, 2022
21f71e6
Fix code style
hannahker Sep 2, 2022
93b05d3
Refactor to move attributes to top level
hannahker Sep 7, 2022
0fff290
Add entry for changelog
hannahker Sep 7, 2022
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
14 changes: 14 additions & 0 deletions src/plots/layout_attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,20 @@ module.exports = {
'between the plotting area and the axis lines'
].join(' ')
},
minreducedwidth: {
valType: 'number',
min: 0,
hannahker marked this conversation as resolved.
Show resolved Hide resolved
dflt: 64,
editType: 'plot',
description: 'Minimum width of the plot with automargin applied.'
},
minreducedheight: {
valType: 'number',
min: 0,
dflt: 64,
editType: 'plot',
description: 'Minimum height of the plot with automargin applied.'
},
Copy link
Contributor

Choose a reason for hiding this comment

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

@alexcjohnson Is there a better place to put these new attributes in?
It looks like these are related to automargin not margin itself.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@archmoj and I discussed this a bit earlier today, and I think the conclusion we came to was: because margin.autoexpand controls whether or not these attributes are even relevant, (1) they should stay here, so they're in the same container as autoexpand, and (2) if autoexpand is false we shouldn't even coerce these two new ones. (The general rule is, if an attribute doesn't do anything because of the value of some other attribute(s), it should not be coerced)

The only question is whether it's really true that these attributes are irrelevant when autoexpand=false. According to its description this would disable automargin from "Legends, colorbars, updatemenus, sliders, axis rangeselector and rangeslider" but what if xaxis.automargin=true? Maybe then the margins can still be pushed even if margin.autoexpand=false? That needs to be tested before we make coercion conditional on margin.autoexpand - but even if that's the case I think it makes sense to leave these attributes where they are.

Copy link
Contributor

Choose a reason for hiding this comment

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

Addressed in 93b05d3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, @archmoj and I looked into the source code and decided that these attributes would be best placed at the top level of layout parameters as they could be related to autoexpand as well (and I think logically make sense alongside height and width).

autoexpand: {
valType: 'boolean',
dflt: true,
Expand Down
14 changes: 6 additions & 8 deletions src/plots/plots.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1512,6 +1512,8 @@ plots.supplyLayoutGlobalDefaults = function(layoutIn, layoutOut, formatObj) {
coerce('margin.b');
coerce('margin.pad');
coerce('margin.autoexpand');
coerce('margin.minreducedwidth');
coerce('margin.minreducedheight');

if(layoutIn.width && layoutIn.height) plots.sanitizeMargins(layoutOut);

Expand Down Expand Up @@ -1861,10 +1863,6 @@ function initMargins(fullLayout) {
var MIN_SPECIFIED_WIDTH = 2;
var MIN_SPECIFIED_HEIGHT = 2;

// could be exposed as an option - the smallest we will allow automargin to shrink a larger plot
var MIN_REDUCED_WIDTH = 64;
var MIN_REDUCED_HEIGHT = 64;

/**
* autoMargin: called by components that may need to expand the margins to
* be rendered on-plot.
Expand All @@ -1889,13 +1887,13 @@ plots.autoMargin = function(gd, id, o) {
var minFinalWidth = Lib.constrain(
width - margin.l - margin.r,
MIN_SPECIFIED_WIDTH,
MIN_REDUCED_WIDTH
margin.minreducedwidth
);

var minFinalHeight = Lib.constrain(
height - margin.t - margin.b,
MIN_SPECIFIED_HEIGHT,
MIN_REDUCED_HEIGHT
margin.minreducedheight
);

var maxSpaceW = Math.max(0, width - minFinalWidth);
Expand Down Expand Up @@ -2032,13 +2030,13 @@ plots.doAutoMargin = function(gd) {
var minFinalWidth = Lib.constrain(
width - margin.l - margin.r,
MIN_SPECIFIED_WIDTH,
MIN_REDUCED_WIDTH
margin.minreducedwidth
);

var minFinalHeight = Lib.constrain(
height - margin.t - margin.b,
MIN_SPECIFIED_HEIGHT,
MIN_REDUCED_HEIGHT
margin.minreducedheight
);

var maxSpaceW = Math.max(0, width - minFinalWidth);
Expand Down
55 changes: 55 additions & 0 deletions test/image/mocks/z-automargin-minreducedheight.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
{
"data": [{
"name": "< D E A T H >",
"marker": { "color": "red" },
"x": [
"Antonio Vivaldi",
"Johann Sebastian Bach",
"Wolfgang Amadeus Mozart",
"Ludwig van Beethoven"
],
"y": [
"1741",
"1750",
"1791",
"1827"
]
},
{
"name": "< B I R T H >",
"marker": { "color": "blue" },
"x": [
"Antonio Vivaldi",
"Johann Sebastian Bach",
"Wolfgang Amadeus Mozart",
"Ludwig van Beethoven"
],
"y": [
"1678",
"1685",
"1756",
"1770"
]
}],
"layout": {
"title": {
"text": "Longest and shortest<br>names and time periods"
},
"width": 240,
"height": 300,
"margin": {
"t": 80,
"b": 20,
"l": 20,
"r": 20,
"minreducedheight": 55
},
"yaxis": {
"type": "date",
"automargin": true
},
"xaxis": {
"automargin": true
}
}
}
56 changes: 56 additions & 0 deletions test/image/mocks/z-automargin-minreducedwidth.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
{
"data": [{
"name": "< D E A T H >",
"marker": { "color": "red" },
"y": [
"Antonio Vivaldi but even longer",
hannahker marked this conversation as resolved.
Show resolved Hide resolved
"Johann Sebastian Bach but even longer",
"Wolfgang Amadeus Mozart but even longer",
"Ludwig van Beethoven but even longer"
],
"x": [
"1741",
"1750",
"1791",
"1827"
]
},
{
"name": "< B I R T H >",
"marker": { "color": "blue" },
"y": [
"Antonio Vivaldi but even longer",
"Johann Sebastian Bach but even longer",
"Wolfgang Amadeus Mozart but even longer",
"Ludwig van Beethoven but even longer"
],
"x": [
"1678",
"1685",
"1756",
"1770"
]
}],
"layout": {
"showlegend": false,
"title": {
"text": "Longest and shortest<br>names and time periods"
},
"width": 240,
"height": 200,
"margin": {
"t": 80,
"b": 20,
"l": 20,
"r": 20,
"minreducedwidth": 55
},
"xaxis": {
"type": "date",
"automargin": true
},
"yaxis": {
"automargin": true
}
}
}