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.

Sign up for GitHub

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

Jump to bottom

Funnel traces #3817

Merged
merged 51 commits into from
May 7, 2019
Merged

Funnel traces #3817

merged 51 commits into from
May 7, 2019

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Apr 29, 2019 •
edited
Loading

This PR added 'bar-like' funnel traces on Cartesian coordinates towards addressing #3504.
Also added

  • new bar and waterfall attribute insidetextanchor ⚓
  • new bar and waterfall attribute textangle, supporting general [-180,180] angles 🚨
  • through Implementation of textinfo for waterfall traces #3790, new waterfall attribute textinfo 📝
  • multiple internal cleanups in bar-like land 🌴

Basic codepen.
React codepen.
@plotly/plotly_js

@etpinard etpinard added this to the v1.48.0 milestone Apr 29, 2019
etpinard
etpinard reviewed Apr 29, 2019
View reviewed changes
src/traces/funnel/plot.js Outdated Show resolved Hide resolved
etpinard
etpinard reviewed Apr 29, 2019
View reviewed changes
src/traces/funnel/style.js
.call(Color.fill, di.mc || cont.color)
.call(Color.stroke, di.mlc || cont.line.color)
.call(Drawing.dashLine, cont.line.dash, di.mlw || cont.line.width)
.style('opacity', trace.selectedpoints && !di.selected ? 0.3 : 1);
Copy link
Contributor

@etpinard etpinard Apr 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For bonus points, as funnel traces have (only) one marker style container, it should be relatively easy to implement [un]selected.marker styles like bar traces have:

plotly.js/src/traces/bar/attributes.js

Lines 199 to 216 in adf9bee

selected: {
marker: {
opacity: scatterAttrs.selected.marker.opacity,
color: scatterAttrs.selected.marker.color,
editType: 'style'
},
textfont: scatterAttrs.selected.textfont,
editType: 'style'
},
unselected: {
marker: {
opacity: scatterAttrs.unselected.marker.opacity,
color: scatterAttrs.unselected.marker.color,
editType: 'style'
},
textfont: scatterAttrs.unselected.textfont,
editType: 'style'
},

Maybe you can come back to when funnelarea is completed.

etpinard
etpinard reviewed Apr 29, 2019
View reviewed changes
src/traces/bar/plot.js
@@ -530,5 +519,32 @@ function calcTextinfo(calcTrace, index, xa, ya) {
if(hasFlag('final')) text.push(formatNumber(final));
}

if(trace.type === 'funnel') {
if(hasFlag('value')) text.push(formatNumber(cdi.s));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a mock testing arrayOk textinfo settings?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No we don't have any.
Should textinfo be arrayOk?
I think in pie it is not. Anyway for the moment arrayOk: false is added here: 8d02bcf for better clarification.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see value in having arrayOk:true for textinfo, but no need to do this in this PR.

etpinard
etpinard reviewed Apr 29, 2019
View reviewed changes
src/traces/bar/plot.js Outdated Show resolved Hide resolved
@archmoj
split handleTickLabelDefaults and split the process into two passes t…
e968da6 
…o coerce prefix and suffix for invisible axes - as required in funnel and waterfall textinfo
@etpinard
Copy link
Contributor

etpinard commented May 7, 2019 •
edited
Loading

@archmoj you're almost there. Here's my list of blocking items:

  • Figure out why the bar_attrs_relative.png baseline changed
  • Make autorange default to 'reversed' on y-axes that only have horizontal funnels
  • Improve the funnel orientation attribute description

@archmoj
Copy link
Contributor Author

archmoj commented May 7, 2019

@archmoj you're almost there. Here's my list of blocking items:

@etpinard Thanks for the review and checklist.

  • Figure out why the bar_attrs_relative.png baseline changed

Good call. Done in 6d50386 so that the none of the current mocks change.

  • Make autorange default to 'reversed' on y-axes that only have horizontal funnels

Done in d0e8fe5.

  • Improve the funnel orientation attribute description

Done in fd1f6ca.

@archmoj
Copy link
Contributor Author

archmoj commented May 7, 2019

@nicolaskruchten @alexcjohnson @etpinard
Please double check funnel and waterfall textinfo print order & naming here:

plotly.js/src/traces/bar/plot.js

Lines 536 to 580 in 6d50386

if(hasFlag('label')) {
text.push(formatLabel(calcTrace[index].p));
}
if(hasFlag('text')) {
tx = Lib.castOption(trace, cdi.i, 'text');
if(tx) text.push(tx);
}
if(trace.type === 'waterfall') {
var delta = +cdi.rawS || cdi.s;
var final = cdi.v;
var initial = final - delta;
if(hasFlag('initial')) text.push(formatNumber(initial));
if(hasFlag('delta')) text.push(formatNumber(delta));
if(hasFlag('final')) text.push(formatNumber(final));
}
if(trace.type === 'funnel') {
if(hasFlag('value')) text.push(formatNumber(cdi.s));
var nPercent = 0;
if(hasFlag('percent initial')) nPercent++;
if(hasFlag('percent previous')) nPercent++;
if(hasFlag('percent total')) nPercent++;
var hasMultiplePercents = nPercent > 1;
if(hasFlag('percent initial')) {
tx = formatPercent(cdi.begR);
if(hasMultiplePercents) tx += ' of initial';
text.push(tx);
}
if(hasFlag('percent previous')) {
tx = formatPercent(cdi.difR);
if(hasMultiplePercents) tx += ' of previous';
text.push(tx);
}
if(hasFlag('percent total')) {
tx = formatPercent(cdi.sumR);
if(hasMultiplePercents) tx += ' of total';
text.push(tx);
}
}

@etpinard
Copy link
Contributor

etpinard commented May 7, 2019

This PR is a triumph 🏆

Not only do we get a new trace type in (i.e. funnel 🌪️ ), but we also get:

  • new bar and waterfall attribute insidetextanchor ⚓
  • new bar and waterfall attribute textangle, supporting general [-180,180] angles 🚨
  • through Implementation of textinfo for waterfall traces #3790, new waterfall attribute textinfo 📝
  • multiple internal cleanups in bar-like land 🌴

@archmoj I know you still have a question about the textinfo behaviour in #3790, so you might want to before wait merging this PR, but regardless

💃 💃 💃 💃 💃 💃 💃

@etpinard
Copy link
Contributor

etpinard commented May 7, 2019

Just to note a few things this PR didn't do:

@etpinard
Copy link
Contributor

etpinard commented May 7, 2019

@nicolaskruchten @alexcjohnson @etpinard
Please double check funnel and waterfall textinfo print order & naming here:

👍 from me.

@archmoj
Copy link
Contributor Author

archmoj commented May 7, 2019

@etpinard Thanks very much for the review and encouragement.
I will move forward and merge this thing into #3790.
There I wait if we need any changes regarding this comment: #3817 (comment).

@archmoj archmoj merged commit 79d6ef1 into fix3777-textinfo-waterfall-fin May 7, 2019
@archmoj archmoj deleted the funnel-traces branch May 7, 2019 19:23
@archmoj archmoj mentioned this pull request May 8, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolaskruchten nicolaskruchten nicolaskruchten left review comments

@alexcjohnson alexcjohnson alexcjohnson left review comments

@etpinard etpinard etpinard left review comments

Assignees
No one assigned
Labels
feature something new
Projects
None yet
Milestone
v1.48.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@archmoj @nicolaskruchten @etpinard @alexcjohnson