-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Unhide overlapping grouped bars within trace #3680
Conversation
- convert sieve instantiation to `new Sieve(traces, {/* */}` signature - more readable barmode switchboard
... under barmode:'group'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@etpinard my questions below are non-blocking to 💃 this awesome fix.
|
||
if(fullTrace.offset === undefined) included.push(calcTrace); | ||
else excluded.push(calcTrace); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we put this logic in a function and reuse it for the case of relative
below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll pass if you don't mind. That would a pretty ugly function (we would have to pass functions setGroupPositionsInGroupMode
, etc) and only save a few lines.
src/traces/bar/cross_trace_calc.js
Outdated
|
||
var sieve = new Sieve(calcTraces, { | ||
separateNegativeValues: barmode === 'relative', | ||
dontMergeOverlappingData: !(barnorm || barmode === 'stack' || barmode === 'relative') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function now has a more clear interface block which is great.
Now that the keys would be presented in the .min
files could we zip separateNegativeValues
into e.g. separateNegVals
and dontMergeOverlappingData
into a short form?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in c019c5c
Thanks for the review @archmoj - I'll until the |
fixes #3651 - where I decided to not add another attribute as the problem described in #3651 only affects bar graphs with
barmode: 'group'
. See #3651 (comment) for more on the topic.cc @plotly/plotly_js @nicolaskruchten