-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Increase axis autorange when bars have outside text labels #7675
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
base: master
Are you sure you want to change the base?
Conversation
55cb0eb to
1264314
Compare
1264314 to
a437597
Compare
e090b3a to
3cb2f47
Compare
camdecoster
left a comment
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 left some syntax and refactoring comments, but this looks good.
| var TEXTPAD = require('./constants').TEXTPAD; | ||
| var BR_TAG_ALL = require('../../lib/svg_text_utils').BR_TAG_ALL; |
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.
| var TEXTPAD = require('./constants').TEXTPAD; | |
| var BR_TAG_ALL = require('../../lib/svg_text_utils').BR_TAG_ALL; | |
| const { TEXTPAD } = require('./constants'); | |
| const { BR_TAG_ALL } = require('../../lib/svg_text_utils'); |
| var nLines = trace.texttemplate | ||
| ? countLines(trace.texttemplate) | ||
| : isArrayOrTypedArray(trace.text) | ||
| ? Math.max(...trace.text.map((t) => countLines(t))) |
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.
Will map work on a typed array?
| if (!text || typeof text !== 'string') return 0; | ||
| return (text.match(BR_TAG_ALL) || []).length + 1; | ||
| } | ||
| var nLines = trace.texttemplate |
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.
| var nLines = trace.texttemplate | |
| const nLines = trace.texttemplate |
| // Yes, I know this looks backwards from what it should be, | ||
| // but it works like this |
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.
Could you reword this? I trust that you're correct, but I don't understand what's happening from your comment.
| trace.textposition == 'outside' && | ||
| (trace.textangle == 'auto' || trace.textangle == 0) |
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.
| trace.textposition == 'outside' && | |
| (trace.textangle == 'auto' || trace.textangle == 0) | |
| trace.textposition === 'outside' && | |
| (trace.textangle === 'auto' || trace.textangle === 0) |
| const extraPad = estimateAxisPaddingForText(fullTrace, calcTrace); | ||
| fullTrace._extremes[sa._id] = Axes.findExtremes(sa, pts, { | ||
| tozero: tozero, | ||
| padded: padded | ||
| padded: padded, | ||
| ppadplus: extraPad.ppadplus, | ||
| ppadminus: extraPad.ppadminus |
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.
| const extraPad = estimateAxisPaddingForText(fullTrace, calcTrace); | |
| fullTrace._extremes[sa._id] = Axes.findExtremes(sa, pts, { | |
| tozero: tozero, | |
| padded: padded | |
| padded: padded, | |
| ppadplus: extraPad.ppadplus, | |
| ppadminus: extraPad.ppadminus | |
| const { ppadminus, ppadplus } = estimateAxisPaddingForText(fullTrace, calcTrace); | |
| fullTrace._extremes[sa._id] = Axes.findExtremes(sa, pts, { | |
| tozero, | |
| padded, | |
| ppadplus, | |
| ppadminus |
| const extraPad = estimateAxisPaddingForText(fullTrace, calcTrace); | ||
| fullTrace._extremes[sa._id] = Axes.findExtremes(sa, pts, { | ||
| tozero: tozero, | ||
| padded: true | ||
| padded: true, | ||
| ppadplus: extraPad.ppadplus, | ||
| ppadminus: extraPad.ppadminus |
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.
| const extraPad = estimateAxisPaddingForText(fullTrace, calcTrace); | |
| fullTrace._extremes[sa._id] = Axes.findExtremes(sa, pts, { | |
| tozero: tozero, | |
| padded: true | |
| padded: true, | |
| ppadplus: extraPad.ppadplus, | |
| ppadminus: extraPad.ppadminus | |
| const { ppadminus, ppadplus } = estimateAxisPaddingForText(fullTrace, calcTrace); | |
| fullTrace._extremes[sa._id] = Axes.findExtremes(sa, pts, { | |
| tozero, | |
| padded: true, | |
| ppadplus, | |
| ppadminus |
| const extraPad = estimateAxisPaddingForText(fullTrace, calcTrace); | ||
| fullTrace._extremes[sa._id] = Axes.findExtremes(sa, pts, { | ||
| // N.B. we don't stack base with 'base', | ||
| // so set tozero:true always! | ||
| tozero: true, | ||
| padded: true | ||
| padded: true, | ||
| ppadplus: extraPad.ppadplus, | ||
| ppadminus: extraPad.ppadminus |
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.
| const extraPad = estimateAxisPaddingForText(fullTrace, calcTrace); | |
| fullTrace._extremes[sa._id] = Axes.findExtremes(sa, pts, { | |
| // N.B. we don't stack base with 'base', | |
| // so set tozero:true always! | |
| tozero: true, | |
| padded: true | |
| padded: true, | |
| ppadplus: extraPad.ppadplus, | |
| ppadminus: extraPad.ppadminus | |
| const { ppadminus, ppadplus } = estimateAxisPaddingForText(fullTrace, calcTrace); | |
| fullTrace._extremes[sa._id] = Axes.findExtremes(sa, pts, { | |
| // N.B. we don't stack base with 'base', | |
| // so set tozero:true always! | |
| tozero: true, | |
| padded: true, | |
| ppadplus, | |
| ppadminus |
| var nLines = trace.texttemplate | ||
| ? countLines(trace.texttemplate) | ||
| : isArrayOrTypedArray(trace.text) | ||
| ? Math.max(...trace.text.map((t) => countLines(t))) | ||
| : countLines(trace.text); |
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.
Could you refactor this to remove the nested ternary?
Closes #2001
Adds a step during bar calc to add extra padding to axis range if bar has outside text labels.
Note: The first commit in this PR is just formatting changes. Use this diff to skip the first commit and see only the meaningful changes.
Codepen here! https://codepen.io/emilykl-code/pen/JoXQqLL
This is a stopgap fix for the fact that text labels are not taken into account during autorange calculations (see #2000).
Unfortunately, as discussed in that issue, modifying autorange to account for text labels is a significant amount of work, so I've opted for a more limited fix to address the issue identified in #2001.
Changes in this PR:
textpositionis'outside',orientationis vertical, andtextortexttemplateis provided and non-falsy, andtextangleis0or'auto'"cliponaxis": "false"from thehist_stackedimage mock, to demonstrate the effect of the changes in this PR (see below)Screenshots
Before:

After:

hist_stackedmock (withcliponaxis: falseremoved) Before:hist_stackedmock (withcliponaxis: falseremoved) After:Steps for testing
Use the following mock:
Verify that it looks like the "before" picture on
masterand the "after" picture on this branch.Other variations to test:
layout.yaxis.autorangeto"reversed"bar.textproperty instead ofbar.texttemplatelayout.barmodeto"stack"You can also load the
hist_stackedmock in the devtools as another test case.