Skip to content

Draw bar group if there is text #3164

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

Closed
wants to merge 5 commits into from
Closed

Draw bar group if there is text #3164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a460735
Draw bar group if there is text
eivindjahren Oct 26, 2018
605c091
Use original coordinates for deciding not to draw bar
eivindjahren Oct 26, 2018
b787713
Update text in bar_attrs_relative
eivindjahren Oct 26, 2018
68e51bc
Fixed json format in bars_attrs_relative
eivindjahren Oct 29, 2018
2dda651
Change single quote to double quote in json
eivindjahren Oct 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions src/traces/bar/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,11 @@ module.exports = function plot(gd, plotinfo, cdbar, barLayer) {
di.ct = [(x0 + x1) / 2, y1];
}

var text = getText(trace, i);
var is_empty = x0 === x1 || y0 === y1;
if(!isNumeric(x0) || !isNumeric(x1) ||
!isNumeric(y0) || !isNumeric(y1) ||
x0 === x1 || y0 === y1) {
(is_empty && !text)) {
bar.remove();
return;
}
Expand Down Expand Up @@ -120,11 +122,13 @@ module.exports = function plot(gd, plotinfo, cdbar, barLayer) {
y1 = fixpx(y1, y0);
}

Lib.ensureSingle(bar, 'path')
.style('vector-effect', 'non-scaling-stroke')
.attr('d',
'M' + x0 + ',' + y0 + 'V' + y1 + 'H' + x1 + 'V' + y0 + 'Z')
.call(Drawing.setClipUrl, plotinfo.layerClipId);
if(!is_empty) {
Lib.ensureSingle(bar, 'path')
.style('vector-effect', 'non-scaling-stroke')
.attr('d',
'M' + x0 + ',' + y0 + 'V' + y1 + 'H' + x1 + 'V' + y0 + 'Z')
.call(Drawing.setClipUrl, plotinfo.layerClipId);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Then we also need to remove the bar if it later disappears. Off your branch I can do:

Plotly.newPlot(gd,
    [{type:'bar',y:[0,1,0,1],text:'hi',textposition:'outside',cliponaxis:false}],
    {width: 400, height: 400})

Which looks great:
screen shot 2018-10-29 at 11 14 56 am
But then if I do:

Plotly.restyle(gd,'y',[[1,0,1,0]])

The newly-removed bars are still there (with their text moved to the right place):
screen shot 2018-10-29 at 11 16 12 am


appendBarText(gd, bar, cd, i, x0, x1, y0, y1);

Expand Down
4 changes: 2 additions & 2 deletions test/image/mocks/bar_attrs_relative.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
"type":"bar"
}, {
"width":[0.4,0.6,0.8,1],
"text":["Three",2,"inside text",0],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I think we need to figure out how to handle these cases. If I put values in where you put "" - "zero" here and "empty" in the last trace - I get the following unfortunate situation:
screen shot 2018-10-29 at 11 23 46 am
I'm not sure why "zero" is showing up below where its bar would have stacked, seems like it should be above and it should push "4" inside the blue bar. "empty" also shows up below where its bar would be if that bar were positive... in an ideal world we might detect that its neighboring bars are negative - in this case only "outside text" to the right, but if it weren't the first bar, perhaps we would require the bar before to also be negative - we treat this as a "negative zero" and move the "empty" label down below the green -1? Anyway that's a pretty weird edge case, I wouldn't worry about it too much, if we could get it to work right with only a "positive zero" I'd be satisfied.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess that the "zero" and " empty" text is showing up near the empty red bars is the new expected behavior. However, textposition: 'auto' is placing 'zero' below the empty bar and not pushing '4' inside. I'll look around and see if the blank-path solution solves this any better.

"text":["Three",2,"inside text",""],
"textposition":"auto",
"textfont":{"size":[10]},
"y":[3,2,1,0],
Expand All @@ -23,7 +23,7 @@
"x":[1,2,3,4],
"type":"bar"
}, {
"text":[0,"outside text",-3,-2],
"text":["","outside text",-3,-2],
"textposition":"auto",
"y":[0,-0.25,-3,-2],
"x":[1,2,3,4],
Expand Down
15 changes: 2 additions & 13 deletions test/jasmine/tests/bar_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1428,20 +1428,15 @@ describe('A bar plot', function() {
text12 = trace1Bar2.querySelector('text'),
trace2Bar0 = getAllBarNodes(traceNodes[2])[0],
path20 = trace2Bar0.querySelector('path'),
text20 = trace2Bar0.querySelector('text'),
trace3Bar0 = getAllBarNodes(traceNodes[3])[0],
path30 = trace3Bar0.querySelector('path'),
text30 = trace3Bar0.querySelector('text');
text20 = trace2Bar0.querySelector('text');

expect(text03.textContent).toBe('4');
expect(text12.textContent).toBe('inside text');
expect(text20.textContent).toBe('-1');
expect(text30.textContent).toBe('outside text');

assertTextIsAbovePath(text03, path03); // outside
assertTextIsInsidePath(text12, path12); // inside
assertTextIsInsidePath(text20, path20); // inside
assertTextIsBelowPath(text30, path30); // outside

// clear bounding box cache - somehow when you cache
// text size too early sometimes it changes later...
Expand Down Expand Up @@ -1491,20 +1486,14 @@ describe('A bar plot', function() {
text12 = trace1Bar2.querySelector('text'),
trace2Bar0 = getAllBarNodes(traceNodes[2])[0],
path20 = trace2Bar0.querySelector('path'),
text20 = trace2Bar0.querySelector('text'),
trace3Bar0 = getAllBarNodes(traceNodes[3])[0],
path30 = trace3Bar0.querySelector('path'),
text30 = trace3Bar0.querySelector('text');
text20 = trace2Bar0.querySelector('text');

expect(text03.textContent).toBe('4');
expect(text12.textContent).toBe('inside text');
expect(text20.textContent).toBe('-1');
expect(text30.textContent).toBe('outside text');

assertTextIsInsidePath(text03, path03); // inside
assertTextIsInsidePath(text12, path12); // inside
assertTextIsInsidePath(text20, path20); // inside
assertTextIsInsidePath(text30, path30); // inside
})
.catch(failTest)
.then(done);
Expand Down