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.

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

texttemplate #3816

Closed
nicolaskruchten opened this issue Apr 29, 2019 · 11 comments · Fixed by #4071
Closed

texttemplate #3816

nicolaskruchten opened this issue Apr 29, 2019 · 11 comments · Fixed by #4071
Assignees
Labels
feature something new
Milestone

Comments

@nicolaskruchten
Copy link
Contributor

This idea keeps coming up as we discuss various questions... Here's an issue for it!

We should have a texttemplate attribute wherever we accept text or textinfo, with similar semantics as hovertemplate.

@antoinerg
Copy link
Contributor

antoinerg commented Jul 12, 2019

Ok so here are the affected traces/components:

$ find . -name "attributes.js" -exec ack -l "\s(textinfo|text):" {} \;
src/traces/box/attributes.js
src/traces/funnel/attributes.js
src/traces/violin/attributes.js
src/traces/pointcloud/attributes.js
src/traces/sunburst/attributes.js
src/traces/funnelarea/attributes.js
src/traces/scatterpolar/attributes.js
src/traces/contourcarpet/attributes.js
src/traces/splom/attributes.js
src/traces/indicator/attributes.js
src/traces/waterfall/attributes.js
src/traces/scatterpolargl/attributes.js
src/traces/mesh3d/attributes.js
src/traces/histogram/attributes.js
src/traces/ohlc/attributes.js
src/traces/scattergeo/attributes.js
src/traces/candlestick/attributes.js
src/traces/cone/attributes.js
src/traces/scattergl/attributes.js
src/traces/surface/attributes.js
src/traces/bar/attributes.js
src/traces/choroplethmapbox/attributes.js
src/traces/pie/attributes.js
src/traces/streamtube/attributes.js
src/traces/volume/attributes.js
src/traces/barpolar/attributes.js
src/traces/scatterternary/attributes.js
src/traces/isosurface/attributes.js
src/traces/choropleth/attributes.js
src/traces/scattercarpet/attributes.js
src/traces/contour/attributes.js
src/traces/densitymapbox/attributes.js
src/traces/scattermapbox/attributes.js
src/traces/scatter/attributes.js
src/traces/scatter3d/attributes.js
src/traces/heatmap/attributes.js
src/components/annotations3d/attributes.js
src/components/colorbar/attributes.js
src/components/annotations/attributes.js

@etpinard
Copy link
Contributor

etpinard commented Jul 12, 2019

More like:

// pie-like traces
src/traces/pie/attributes.js
src/traces/sunburst/attributes.js
src/traces/funnelarea/attributes.js

// bar-like
src/traces/bar/attributes.js
src/traces/funnel/attributes.js
src/traces/waterfall/attributes.js

// scatter-like
src/traces/scatter/attributes.js
src/traces/scatterpolar/attributes.js
src/traces/scatterpolargl/attributes.js
src/traces/scattergeo/attributes.js
src/traces/scattergl/attributes.js
src/traces/scatterternary/attributes.js
src/traces/scattercarpet/attributes.js
src/traces/scattermapbox/attributes.js
src/traces/scatter3d/attributes.js

// not sure ?
src/traces/indicator/attributes.js

A lot of traces have text as simply an alias for hovertext, so they don't need textinfo.

I don't think textinfo is necessary for components like annotations as they don't have any data arrays (e.g. %{y}) associated with them.

@etpinard
Copy link
Contributor

... and we can probably do 👌 w/o adding texttemplate for all the scatter* traces in the first iteration.

So that leaves 6 trace types: pie, sunburst, funnelarea, bar, waterfall and funnel.

@antoinerg
Copy link
Contributor

In the context of hovertemplate, should the variable text be given the value of the compiled texttemplate when it's available or should it only point to the value in text?

@etpinard
Copy link
Contributor

Thanks for pointing this out!

should the variable text be given the value of the compiled texttemplate when it's available or should it only point to the value in text?

I think pointing to the value in text will be sufficient. That is,

{
  text: '%{label} %{value}',
  hovertemplte: '%{text}'
}

should show %{label} %{value} (i.e. the raw un-templated string), as it would be easy enough to set:

{
  text: '%{label} %{value}',
  hovertemplte: '%{label} %{value}'
}

We had a similar discussion in #3865

@etpinard
Copy link
Contributor

More precisely in #3865 (comment)

@antoinerg
Copy link
Contributor

... and we can probably do w/o adding texttemplate for all the scatter* traces in the first iteration.

So that leaves 6 trace types: pie, sunburst, funnelarea, bar, waterfall and funnel.

Initial implementation for those 6 traces on this branch: https://github.com/plotly/plotly.js/compare/texttemplate

Note that I'll remove texttemplate_tests.js and insert the tests into each trace's test file.

@etpinard
Copy link
Contributor

That's a very 💪 first attempt @antoinerg !

I like the way you put some of the logic in Drawing.textPointStyle. So DRY!

A few comments:

  • now that trace._meta is handled in Drawing.textPointStyle, we could potentially cut a few lines like this block

if(trace._meta) {
txt = Lib.templateString(txt, trace._meta);
}

see #3865 for more potentially redundant trace._meta blocks.

  • We'll have to rename Lib.hovertemplateString to e.g. Lib.traceTemplateString or Lib.templateStringWithData or maybe we could combine it Lib.templateString?

  • We should make sure traces that have textinfo and texttemplate do not coerce textinfo when texttemplate is non-empty, similar to

if(!traceOut.hovertemplate) Lib.coerceHoverinfo(traceIn, traceOut, layout);

  • You wrote "TODO: calcTextinfo should build a texttemplate pass it to calcTexttemplate()". That's interesting, I was thinking about doing the same in our _module.hoverPoints methods to DRY things up. But we should be careful here. creating a template string than parsing it must be slower than simply creating a string from (text|hover)info flags. How much slower, I don't know. It would be interesting to benchmark this e.g. try plotting a trace with 1e4 with a texttemplate and check how much slower that is compared to with just a text array.

  • The jasmine tests you wrote look good, but I would prefer seeing texttemplate in action in one or multiple mocks.

@antoinerg
Copy link
Contributor

antoinerg commented Jul 17, 2019

  • We'll have to rename Lib.hovertemplateString to e.g. Lib.traceTemplateString or Lib.templateStringWithData or maybe we could combine it Lib.templateString?

I think it should have template, string and format in its name. Its main difference with Lib.templateString is that it supports number formatting (as well as default formats). So, what about Lib.templateFormatString?

cc @etpinard

@etpinard
Copy link
Contributor

So, what about Lib.templateFormatString?

Lib.templateFormatString sounds 10/10 to me

@antoinerg
Copy link
Contributor

antoinerg commented Jul 19, 2019

So, what about Lib.templateFormatString?

Lib.templateFormatString sounds 10/10 to me

Done in branch texttemplate

To do:

  • fix color variable in pie and funnelarea
  • break down texttemplate_test.js into each trace's test file
  • create Lib.templateFormatString
  • create a mock/baseline
  • do not coerce textinfo when texttemplate (without tests for now)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants