Skip to content

Bug fix - do not display empty bars when line.width is zero - #4056

Merged
archmoj merged 4 commits into
masterfrom
fix4047-bar-size-and-width-zero
Jul 18, 2019
Merged

Bug fix - do not display empty bars when line.width is zero#4056
archmoj merged 4 commits into
masterfrom
fix4047-bar-size-and-width-zero

Conversation

@archmoj

@archmojarchmoj commented Jul 17, 2019

Copy link
Copy Markdown
Contributor

Fixes#4047.
Empty bar and funnel rectangles should not be displayed when marker.line.width is set to zero.

Codepen after

Also addressing #4057 for bars in this PR.
Codepen after

@plotly/plotly_js

Comment threadsrc/traces/bar/plot.js Outdated
"type": "bar",
"marker": {
"line": {
"width": [10, 0, 10, 0, 10]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like this mock only has array marker.line.width value. Could you add one trace that test scaler marker.line.width?

Maybe you can leave this mock alone and simply add another mock using https://codepen.io/MojtabaSamimi/pen/bXbmba , so that we also test the relative-stacked case.

@etpinard

Copy link
Copy Markdown
Contributor

Thanks for taking this one @archmoj - I made two minor comments.

move getLineWidth function to helpers and reuse it in hover
also fixing 4057
@archmoj

Copy link
Copy Markdown
ContributorAuthor

@etpinard Thanks for the review!
This is now ready for the second round.
As I updated the PR description please note that the #4057 is also addressed for bars.

Comment threadsrc/traces/bar/plot.js
Comment threadsrc/traces/bar/helpers.js Outdated
return value;
};

exports.getLineWidth = function(trace, di) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bars don't have line.width, but still calling this getMarkerLineWidth would be better.

Comment threadsrc/lib/index.js Outdated
Comment threadsrc/lib/index.js
for(var i = 0; i < imax; i++) cd[i][cdAttr] = isNumber ? +traceAttr[i] : traceAttr[i];
for(var i = 0; i < imax; i++) {
var v = traceAttr[i];
cd[i][cdAttr] = hasFn ? fn(v) : v;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder what's faster

varhasFn=typeoffn==='function';for(vari=0;i<N;i++){arrayOut[i]=hasFn ? fn(arrayIn[i]) : arrayIn[i];}

or

fn=typeoffn==='function' ? fn : Lib.noop;for(vari=0;i<N;i++){arrayOut[i]=fn(arrayIn[i]);}

No need to change anything, I'm just thinking out-loud.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Interesting question. And I like the minimalist version with Lib.noop!
That may depend on the browser.
Having two loops may also be fast.

vari=0;if(typeoffn==='function'){for(;i<N;++i)arrayOut[i]=fn(arrayIn[i]);}else{for(;i<N;++i)arrayOut[i]=arrayIn[i];}

@etpinard

Copy link
Copy Markdown
Contributor

Nicely done 💃

Thanks for the fixes!

@archmoj
archmoj merged commit 9927079 into masterJul 18, 2019
@archmoj
archmoj deleted the fix4047-bar-size-and-width-zero branch July 18, 2019 18:59
@archmojarchmoj added this to the v1.49.0 milestone Jul 23, 2019
@etpinardetpinard mentioned this pull request Sep 11, 2019
6 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugsomething broken

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Extra bar traces show up in stacked bar chart in relative barmode in >=1.48.0

2 participants

@archmoj@etpinard