-
-
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
Add texttemplate
attribute to shape.label
#6527
Conversation
Currently working for "easy" cases. Edge cases I haven't tackled yet:
|
Here is a codepen demonstrating the current behavior: https://codepen.io/emily_plotly/pen/eYLrKNV |
Nice! If it's easy there are a few more that occur to me might be useful. We can leave them for later too though:
Let's not support paths for now.
Seems like we should handle absolute and relative differently here: for Makes me think about log axes though - seems like if you've drawn a line on a log axis the only meaningful slope is also in log units. The use cases I've seen for this:
So
JS doesn't have those 😉 > 1/0
Infinity
> 0/0
NaN If the result is we just show those strings with no further formatting, that's fine IMO
I guess it's the aspect ratio for those shapes. Odd use case but probably easy to just allow it. |
Thanks @alexcjohnson !
These should all be pretty easy. For the first two, would
Fine with me. How should we handle the case where a user sets a |
yes 😁
Do whatever we do today if you make a |
Instead of On another note, perhaps Also concerning |
Ah interesting -
Let’s stay away from paths please. It’s going to be a rabbit hole dealing with straight lines, arcs, and two orders of Bézier curves, plus open and closed paths, and nobody has asked for it. If and when we do that we can add |
Sure, I can see an argument for having both Is there ever an instance where the x and y axes are swapped, i.e. y is horizontal and x is vertical? That would be weird but just double-checking whether I can't think of a use-case for |
Very cool discussion going on!
Why would you opt for milliseconds? I know that in many programming language including JavaScript milliseconds is the default timedelta unit. However in Python it is the much human friendly second 😄 Would it add too much complexity to have different units for plotly.js and plotly.py or just use seconds all together? Also since it is only for displaying we dont encounter floating point arithmetic issues. @emilykl Could you please update the pull request description, whenever we finalized the set of supported variables. Also it might be worthwhile to split the pull request into the common and not so common cases, but I dont have a strong opinion about this 😄 This may include log axis support, since although the use case you describe seams really elegant I dont know how often this is actually is needed. I would prefer |
@JulianWgs Thank you, great to have your feedback. For the final list of variables, @archmoj are you okay with the following? If there's any dispute about any of these I'd argue for leaving them out of this PR rather than prolonging the discussion, since it's trivial to add more variables later.
Regarding time units, I believe Plotly.js uses milliseconds internally for datetime representations, and using a different unit only for shape labels has the potential to create some confusion -- I will verify internally though whether there's precedent for this question. @archmoj @alexcjohnson Would it be reasonable to exclude slope calculations for log axes for the moment? Unless you think the correct slope is obvious in all cases and it's a very simple implementation. |
Let's leave
Finally instead of |
Let’s keep Personally I like To me, the behavior on log axes is clear (one unit of difference on a log scale is one power of ten in the data, again the same as our internal representation), but since nobody is requesting it right now we can leave it for later. Re: milliseconds - we have a bunch of places in plotlyjs where time differences are already specified in milliseconds, so I don’t think it’s a good idea to change just this one. At some point we could try to add a units field somewhere, then people could specify seconds, days, or whatever. But absent that I think we need to leave it as milliseconds. |
I would always prefer English words over mathematical or software expressions (but for example dx and dy are fine), but this might be due to my Python background.
Slope is a must for our use case. @emilykl Everything else on your list is fine for me as well. I haven't checked the code yet. Have you started the implementation? If we need further discussion on naming I would propose not to wait for the discussion to end, but start implementing as soon as possible :) |
@JulianWgs They are almost all implemented -- I should be able to finish the remaining ones today ;) |
@emilykl the PR is looking good now. |
// If no label text or texttemplate, return | ||
if(!(options.label.text || options.label.texttemplate)) return; | ||
|
||
// Text template overrides 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.
Let defaults
do the overriding - ie don't coerce text
if there's a 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.
and even better - don't try to coerce texttemplate
if type==='path'
- then you don't need the if(options.type !== 'path')
below :)
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 see the comment below about new shapes - you may be right that the if
below is still needed, but it'll still be better if we only coerce what's needed.
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 Changed the coercion in defaults.js
but I believe we still need the if(texttemplate)
statement (see updated code) -- correct me if wrong!
f949a5d
to
66a12de
Compare
"editable": true, | ||
"path": "M3.71,0.75L2.26,3.99L3.71,3.99Z", | ||
"layer": "above" | ||
}, |
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.
OK. Please update the baseline so that image test would pass: https://app.circleci.com/pipelines/github/plotly/plotly.js/8520/workflows/491afb80-1892-4f25-8f62-b5b25db1bfb4/jobs/188422/artifacts
@alexcjohnson @archmoj I think this PR is finally ready! I've addressed all above conversations -- let me know if there's anything additional remaining. I added a section in the PR description to explain the behavior for |
All is looking ⭐ good ⭐ to me. |
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.
💃 Fantastic work @emilykl!
🎉 🎉 Thanks @archmoj and @alexcjohnson! 🎉 🎉 |
Hey thanks for this Future, i try to have something that can calculate percentage distance, so simply draw a rectangle, than ( (y0 / y1) -1 ) *100 , can we intigrate something like this, or enable to do math direct in the texttemplate. Or did somebody know of a other way for it? |
@mosaikme arbitrary math was not implemented but was discussed in the original issue #6511 (comment) - feel free to open a new issue to request this, or even better a PR to make it happen 😉 |
ok , sad. I will try to add this. But where to start? Would it be enough to edit or add percent calculation here https://github.com/plotly/plotly.js/blob/master/src/components/shapes/label_texttemplate.js as a new function , and than add it the module.export?
|
Resolves #6511
Add
texttemplate
attribute toshape.label
andnewshape.label
.texttemplate
supports insertion of shape variables using"%{variable}"
syntax, with d3 number and date formatting. A single multiplication or division operation is also supported, to allow for unit conversions. Shape label displays correct values for variables used intexttemplate
, and update values live as shape is moved or resized.Supported variables:
x0
x1
y0
y1
Calculated variables:
xcenter
(calculated as(x0+x1)/2
)ycenter
(calculated as(y0+y1)/2
)dx
(calculated asx1-x0
)dy
(calculated asy1-y0
)width
(calculated asabs(x1-x0)
)height
(calculated asabs(y1-y0)
)length
(calculated assqrt(dx^2+dy^2)
) -- for lines onlyslope
(calculated as(y1-y0)/(x1-x0)
)Behavior for
log
,date
, andcategory
axes:date
axes and strings forcategory
axes)log
axes, milliseconds fordate
axes, integers forcategory
axes)xcenter
andycenter
are converted back to data values after calculationThis PR does not cover rendering of the shape text during mousedown of the initial draw -- due to additional complexity, that will be covered in a separate issue, captured by #6540.
API
shape.label.texttemplate
: Template string used for rendering the new shape's label. Note that this will overridetext
. Variables are inserted using"%{variable}"
, for example"x0: %{x0}"
. Numbers are formatted using d3-format's syntax"%{variable:d3-format}"
, for example"Price: %{x0:$.2f}"
. See https://github.com/d3/d3-format/tree/v1.4.5#d3-format for details on the formatting syntax.Dates are formatted using d3-time-format's syntax
"%{variable|d3-time-format}"
, for example"Day: %{x0|%A}"
. See: https://github.com/d3/d3-time-format/tree/v2.2.3#locale_formatA single multiplication or division operation may be applied to numeric variables, and combined with d3 number formatting, for example
"Length in cm: %{x0*2.54}"
,"%{slope*60:.1f} meters per second."
For log axes, variable values are given in log units. For date axes, x/y coordinate variables and center variables use datetimes, while all other variable values use values in ms.
Finally, the template string has access to variables
x0
,x1
,y0
,y1
,slope
,dx
,dy
,width
,height
,length
,xcenter
andycenter
.Partnership
Development of this feature is sponsored by Volkswagen's Center of Excellence for Battery Systems.