-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Support adding text labels to lines and shapes #6454
Conversation
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.
Very nice work.
Thanks @emilykl 🏆
Please find my first round comments & suggestions below.
src/components/shapes/attributes.js
Outdated
'bottom left', 'bottom center', 'bottom right', | ||
'top start', 'top end', | ||
'middle start', 'middle end', | ||
'bottom start', 'bottom end', |
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.
Are these start|end
really necessary?
If so, let's describe them in the description please.
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.
Definitely open to suggestions here, but my thought was that corner-based positions like top right
, top left
etc. don't really make sense for lines because imagine e.g. a downward sloping line -- top right
of the bounding box would place the label at a far-off point in space nowhere close to the line.
Even if the line started out sloping upward and so top right
made sense, it could be dragged by the user into a downward-sloping position.
I agree that mixing start|end
positions with top|middle|bottom
is a little convoluted though. One thought I had was that maybe lines should only support 4 specific label positions: top
, bottom
, start
and end
(or potentially, top
, bottom
, left
and right
). Not sure if that would be more or less confusing.
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.
Maybe @alexcjohnson has thoughts here as well?
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.
My thinking about these attributes is that position
refers to a single point on the shape, and [xanchor, yanchor]
(plus padding
) refers to a single point on the text element, and you position the text so that those two points are in the same place.
So for lines, the only position
values that make sense are start|middle|end
. We can set smart defaults for xanchor
based on those ({start: 'left', middle: 'center', end: 'right'}
so that by default for position: start|end
the start or end of the text aligns with the start or end of the line, and for position: middle
the text is centered on the line. Then yanchor: bottom
puts the text on top of the line (ie the bottom of the text is on the line), yanchor: top
puts the text under the line, and yanchor: 'middle'
puts the text on top of the line. You can get other cool effects then like text preceding the line (position: start, xanchor: right, yanchor: middle
) or following the line (position: end, xanchor: left, yanchor: middle
)
For other shapes the first nine positions you have are good, and in those cases we can set smart defaults for both xanchor
and yanchor
to match, ie [xanchorDefault, yanchorDefault] = position.split(' ')
so (for rects anyway) by default the text is drawn just inside whatever corner or side you specify with position
. Default for all these shapes should be middle center
though.
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.
Yeah I think this makes sense @alexcjohnson , thanks!
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 I've implemented these changes to the position
property for lines
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 Oh, I realized I skimmed over this part of your comment before:
For other shapes the first nine positions you have are good, and in those cases we can set smart defaults for both xanchor and yanchor to match, ie [xanchorDefault, yanchorDefault] = position.split(' ') so (for rects anyway) by default the text is drawn just inside whatever corner or side you specify with position.
I've implemented the exact opposite -- by default the text is drawn outside the corner, if xanchor
and yanchor
are not specified. That feels more intuitive to me, and I think it generally looks better, so wanted to verify that that's okay. If it needs to be the other way for consistency with other parts of the API, I can change it.
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 guess it depends what we consider the default or most common use case for labels on shapes. I feel like if I was using a shape to label a particular region of a graph - an important x or y range, typically - I'd likely want the label within that region, but off in a corner so it avoids most of the data in that region.
I guess that really only applies to rectangles, for circles or paths being outside the region would be better as otherwise the label would often overlap the edge of the shape, and there are other use cases even for rects where an outside label would make more sense. Perhaps though even then it would make sense for xanchor
to be within the bounds and just yanchor
to be outside? So for example textposition="top left"
would default to xanchor="left", yanchor="bottom"
?
Also: can we handle this logic at the defaults
stage instead of the draw
stage? If we do that, the choices we made show up in fullLayout
which can help some users understand and fix unwanted behavior. And at that point I don't see a use for "auto"
in these attributes (which in turn means they can probably stop inheriting from annotations)
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.
Oh I guess xanchor
on lines with textposition='start'|'end'
still needs 'auto'
because it depends whether the start is toward the left or right, and we can't know that unless we also know the axis range it's on. That doesn't apply to yanchor
though, I still don't see a reason to let that be 'auto'
.
But a little more logic around this perhaps, just for lines: If textangle='auto'
I would expect the text to be along the line by default, rather than extending off the end. Whereas for any other textangle
I'd expect the text to extend off the end, since if it tried to go along the line it would often overlap the line.
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 I've updated xanchor
and yanchor
defaults so they reflect the following behavior:
yanchor
:- Default for lines is
bottom
- Default for all other shapes is such that the label is positioned inside the shape bounding box (e.g. if
textposition
istop right
,yanchor
istop
. auto
is not a supported value foryanchor
- Default for lines is
xanchor
default is alwaysauto
. This has the following behavior at render:- For lines:
- If
textangle
isauto
(parallel to line)xanchor
is set so that text is inside the line bounding box - If
textangle
is anything else,xanchor
is set in the opposite direction, so that text is outside the line bounding box
- If
- For other shapes:
xanchor
is set such that the label is positioned inside the shape bounding box (e.g. iftextposition
istop right
,xanchor
isright
- For lines:
This mostly works pretty nicely. The only cases that are not ideal are circles and paths -- since the text is positioned inside the bounding box, it overlaps the circle/path in an odd way for any position besides middle center
.
We could potentially position the text outside the bounding box by default for circles and paths... but not sure whether that's worth the additional complexity.
}), | ||
position: { | ||
valType: 'enumerated', | ||
values: [ |
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.
It looks like we need to have auto
dflt
and value
in respect to src/components/shapes/defaults.js
logic.
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.
@archmoj What would be the use case for an auto
value for textposition
?
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.
@archmoj What would be the use case for an
auto
value fortextposition
?
Hmm.. that's a good idea. But as far as I recall we don't have such an option for textposition
in other places. So perhaps we could keep this part as is.
Thanks so much for the first round review @archmoj ! |
@alexcjohnson @archmoj A couple dangling property naming questions:
|
src/components/shapes/attributes.js
Outdated
description: [ | ||
'Sets the position of the label text relative to the shape.', | ||
'Supported values for rectangles, circles and paths are', | ||
'`top left`, `top center`, `top right`, `middle left`,', |
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.
Please note that back quotes are used around attribute names not the values.
In all the new documentation, please place values inside *
instead.
This line for example would become:
'*top left*, *top center*, *top right*, *middle left*,',
We are almost there. FYI - These files would be used during the release process to generate the changelog e.g. https://github.com/plotly/plotly.js/releases/tag/v2.17.0 Please see https://github.com/plotly/plotly.js/blob/master/draftlogs/README.md for more info. |
@emilykl The PR initial description (#6454 (comment)) looks great! Is it up-to-date too? |
Excellent PR. Thanks @emilykl 🏆 🎖️ 🏅 |
@archmoj Just updated the PR description! |
Resolves #6430
Adds a
label
property to Shapes allowing users to define text labels associated with any shape.label
property has sub-propertytext
to set the label text and other properties for setting position, styling, angle, etc. Label moves with shape during shape drag and plot drag.shape.label API
label
: Top-level property to be added toshape
, holding all label optionstext
: The label’s textfont
: The label’s font properties (same properties as forannotation
)color
family
size
textposition
: The label’s position relative to the shape.[ start | middle | end ]
. Default:middle
[ top | middle | bottom ] [ left | center | right ]
. Default:middle center
textangle
: Rotation angle of the label.auto
(same angle as line). Default:auto
xanchor
: The x-component of the anchor point on the label used to determine position (in the pre-rotated reference frame). Possible values :[ auto | left | center | right ]
. Default:auto
, which has the following behavior —center
if position iscenter
, otherwise whatever places the label furthest towards the inside of the shape (e.g. if position istop right
,xanchor
defaults toright
)yanchor
: The y-component of the anchor point on the label used to determine position (in the pre-rotated reference frame). Possible values :[ top | middle | bottom ]
. Default:bottom
for lines.middle
for all other shapes if position ismiddle
, otherwise whatever places the label furthest towards the inside of the shape (e.g. if position istop right
,yanchor
defaults totop
)padding
: Offset of label relative toxanchor
andyanchor
(symmetrical on all sides)xanchor
iscenter
yanchor
ismiddle
auto
:padding
is treated as a perpendicular offset from the lineRemaining loose ends (all resolved)
Heads up @archmoj @alexcjohnson -- I'll need help tying up these items.Label position update during drag does not work wheneditable=True
is set for individual shapes. (It does work wheneditable=True
is set for entire plot.)Addinglabel
properties todraw_newshape
?Some Jasmine tests are timing out -- having a hard time debugging theseFeedback on image baselinesGeneral feedback and suggestionsPartnership
Development of this feature is sponsored by Volkswagen's Center of Excellence for Battery Systems.