-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
bar transitions #4180
bar transitions #4180
Conversation
@@ -51,7 +51,28 @@ function getXY(di, xa, ya, isHorizontal) { | |||
return isHorizontal ? [s, p] : [p, s]; | |||
} | |||
|
|||
function plot(gd, plotinfo, cdModule, traceLayer, opts) { | |||
function transition(selection, opts, makeOnCompleteCallback) { |
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.
Clean wrapper!
It's a little different than what scatter/plot.js
does, in particular this piece:
plotly.js/src/traces/scatter/plot.js
Lines 67 to 73 in 46866fd
transition.each(function() { | |
// Must run the selection again since otherwise enters/updates get grouped together | |
// and these get executed out of order. Except we need them in order! | |
scatterLayer.selectAll('g.trace').each(function(d, i) { | |
plotOne(gd, i, plotinfo, d, cdscatterSorted, this, transitionOpts); | |
}); | |
}); |
@antoinerg have you noticed "out of order" transitions?
Your wrapper plays better with Lib.makeTraceGroups
, so if there are no side-effects, maybe we could try this pattern in scatter/plot.js
and maybe fix #3931 while at it.
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.
One drawback from your pattern:
- we have to pass
opts
andmakeOnCompleteCallback
around in all thetransition
calls
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.
scatter/plot.js
uses this trick:
plotly.js/src/traces/scatter/plot.js
Lines 126 to 128 in 46866fd
function transition(selection) { | |
return hasTransition ? selection.transition() : selection; | |
} |
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.
This comment is non-blocking
@@ -325,9 +354,12 @@ function appendBarText(gd, plotinfo, bar, calcTrace, i, x0, x1, y0, y1, opts) { | |||
(textPosition === 'outside') ? | |||
outsideTextFont : insideTextFont); | |||
|
|||
var currentTransform = textSelection.attr('transform'); | |||
textSelection.attr('transform', ''); | |||
textBB = Drawing.bBox(textSelection.node()), |
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.
So here, you trying to get the "untransformed" bounding box, correct? Nice catch!
I guess that's what we have to do in the current bar text svg structure:
where the transform is attached to the <text>
element.
Compare this to scatter text:
which then can benefit from this piece of logic in Drawing.bBox
:
plotly.js/src/components/drawing/index.js
Lines 921 to 934 in 46866fd
if(!transform) { | |
// in this case, just varying x and y, don't bother caching | |
// the final bBox because the alteration is quick. | |
var innerBB = drawing.bBox(innerNode, false, hash); | |
if(x) { | |
innerBB.left += x; | |
innerBB.right += x; | |
} | |
if(y) { | |
innerBB.top += y; | |
innerBB.bottom += y; | |
} | |
return innerBB; | |
} |
and not have to worry about removing and adding back the transform.
Oh well, @antoinerg nothing to do here for now, I just wanted to write this down for future reference.
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.
This comment is non-blocking
"height": 400, | ||
"yaxis": {"range": [0, 30]} | ||
}, | ||
"frames": [ |
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.
Awesome PR @antoinerg !! |
86ba54a
to
16c88f8
Compare
@@ -442,7 +442,7 @@ drawing.singlePointStyle = function(d, sel, trace, fns, gd) { | |||
fill: 'none' | |||
}); | |||
} else { | |||
sel.style('stroke-width', lineWidth + 'px'); | |||
sel.style('stroke-width', (d.isBlank ? 0 : lineWidth) + 'px'); |
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.
@antoinerg please don't do that again on branches associated with open PRs. This makes the |
Awesome work @antoinerg - this PR is a huge step forward in transitions land. As we add more 💃 💃 💃 |
Introduces transitions for
bar
and closes #3896 . The following can now be tweened:Codepen showcasing different mocks
Codepen showcasing that all
barmode
work properlyCodepen showing transition to/from 0 (ie.
isBlank
)checkTransition
Another notable addition in this PR is
checkTransition
which facilitate the testing of transitions. By hijackingDate.now()
, it is possible to change and freeze time in order to inspect the resulting SVG at any moment during a transition. This way, we can guarantee that tweening works on the intended elements. See 8166eb7 for details.