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.
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
Mult axis formatting #6334
Mult axis formatting #6334
Changes from all commits
e4a81e3
de79261
18f21cb
0e85776
ba0c587
2731eb8
c535c56
a8fcadc
0f93feb
d79ef96
b1d10c5
3fa0f0c
50fe1fd
1984b7e
b9c77b8
8a420d6
8d0c9eb
5528a35
fbdc5a4
d742cc5
c2ba6b0
cb66b12
38f17b2
bbf682f
0175979
7a1c9d8
5aee58b
fd098a9
79c2c74
bc9bfa2
5ffbdd2
168b3db
472efb9
0c33429
83e7f82
6e71f53
28eadb0
6d5ecb5
2b951fd
f97f27c
de4dd05
38d3e71
683d0f3
eca5d4f
b84f89d
5f2240f
81b661f
ec1f855
e56020d
b8bfab5
b41678c
6064e5a
085c2b7
49ddf46
a45eca6
56605bc
5506ed4
61c2889
d06bc6d
bab8020
09b5d0c
b89f59a
948809b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
What happens / should happen if one sets
shift
to zero?Can't we use zero as default instead of false?
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.
shift=0
will cause the axis to directly overlap the one that is anchored to the graph on the same size, which is the same as the defaultshift=false
. I do thinkshift=false
is more intuitive as the default as it's likely that most people will useshift=true
-- the numeric values are for rarer cases with really fine-tuned positioning needs.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.
Numbers and
“auto”
(rather thantrue
) is a pattern we’ve used several other placesThere 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.
If the default off is "false", would it be clearer for the 'on' to be "true"? But happy to switch to "auto" if that's better aligned with the existing standard.
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.
Right, the idea would be to not allow
true
andfalse
, just numbers and'auto'
, but with a default of0
.Numbers plus
'auto'
is what we do for example with tickangle (valType: 'angle'
is a number with special behavior to wrap larger values into[-180, 180]
), and font size and a couple of other places.On the other hand, in all those cases
'auto'
is the default, whereas here we want0
to be the default. As you have it now, this requiresshift
to havevalType: 'any'
which isn't great, it really should bevalType: 'number'
. We could addextras
like we have forflaglist
attributes (actually I thought we already had this, but apparently not), or a more targeted option likeautoOk: true
to just allow'auto'
. If we do this though we need matching logic in the plotly.pyNumberValidator
That's a bit annoying, though I suspect unless we do that there's a subtle bug with the above number-with-default-auto cases: if you set a non-auto value in a template you won't be able to get back to auto later. Or even just if you've set a value in your figure and you want to unset it later, or explicitly affirm the value
'auto'
.I do like the idea of
shift='auto'
orshift=<some number>
, I think it's easy to document, intuitive to read if not guessable without the docs, and compact. But if we want a solution that doesn't involve modifying the behavior ofcoerce
we can either:valType: 'any'
for the moment, and change it in a follow-up PR when we have more time to devote to coerce and the plotly.py validatorsshift
withvalType: 'number'
andautoshift
withvalType: 'boolean'
(and iftrue
we don't coerceshift
).@hannahker @archmoj thoughts?
We do need to update
AngleValidator
regardless - it doesn't supportarrayOk
as we discussed recently on slack and also seems to not handle an explicit'auto'
(cc @nicolaskruchten)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 think we should possibly make this attribute an object then provide various current/future options inside it e.g.
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.
@alexcjohnson @archmoj so we've settled on an API with two params:
shiftauto
: boolean where you turn on/off the automatic repositioning of axes that would be overlapping. Default = 'false'shiftval
: numeric where you indicate the number of pixels that you want to move the axis from where it would otherwise be. Default =0
.Is this correct?
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.
(already posted this on slack, but repeating so it's visible to all)
OK after discussing further, there's a nice reason to make these separate attributes: so when the axis is auto-shifted, you can use the shift parameter to control the extra spacing between subsequent axes.
autoshift
: boolean, defaultfalse
shift
: number, nodflt
defined in the attribute itself, but the dynamic (and described in the attribute description) default is0
whenautoshift===false
and3
(yes +/-3 I guess, depending on the side) ifautoshift===true