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

Fix - hide empty bars in interactive mode #4522

Merged
merged 4 commits into from
Jan 29, 2020
Merged

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Jan 28, 2020

Fixes #4499

before vs after

@plotly/plotly_js

@archmoj archmoj added bug something broken status: reviewable labels Jan 28, 2020
src/traces/bar/plot.js Outdated Show resolved Hide resolved
@etpinard
Copy link
Contributor

etpinard commented Jan 29, 2020

Thanks @archmoj for the quick fix. After some testing, looks like the behaviour is now correct. I am a little worried though about how complex the logic in bar/plot.js has become.

At the very least, we should add two new variables to make the logic easier to digest:

  • one for isHorizontal && (x1 - x0 !== 0) e.g. setting a variable named spanHorizontal
  • one for !isHorizontal && (y1 - y0 !== 0) e.g. " " " " spansVertical

which could be in

            // display zeros if line.width > 0
            if(isBlank && shouldDisplayZeros && helpers.getLineWidth(trace, di) && (isHorizontal ? x1 - x0 === 0 : y1 - y0 === 0)) {
                isBlank = false;
            }

and below in

if(!gd._context.staticPlot) {
  // ...

  if(spansHorizontal) {
                x0 = fixpx(x0, x1);
                x1 = fixpx(x1, x0);
  }
  if(spansVertical) {
                y0 = fixpx(y0, y1);
                y1 = fixpx(y1, y0);
  }
}

instead of monkey-patching the expandVisible helper for the degenerate case.


Does that sound ok to you @archmoj ?

@archmoj
Copy link
Contributor Author

archmoj commented Jan 29, 2020

@etpinard that makes sense.
Alternatively we may consider adding "only" this line:

if(v === vc) return v;

to expandToVisible function without adding extra logic to bar.

@etpinard
Copy link
Contributor

etpinard commented Jan 29, 2020

Alternatively we may consider adding "only" this line:

if(v === vc) return v;

Hmm. Wouldn't that defeat the purpose of expandVisible? Right now, if v === vc, v gets floored or ceiled.

@etpinard
Copy link
Contributor

etpinard commented Jan 29, 2020

... and unfortunately we can't rely on the image tests here as this piece of logic in inside a !gd._context.staticPlot block.

@archmoj
Copy link
Contributor Author

archmoj commented Jan 29, 2020

OK. Addressing your #4522 (comment) sounds the safest fix.

@etpinard
Copy link
Contributor

Yep, that's a step in the right direction - thanks 💃

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

Successfully merging this pull request may close these issues.

Null values show up as thin bars in Bar chart if you set an axis range including 0.
2 participants