Remove zoomable animation on Math #696
Conversation
🦋 Changeset detectedLatest commit: a561503 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (99f27d3) and published it to npm. You Example: yarn add @khanacademy/perseus@PR696 |
|
Size Change: +55 B (0%) Total Size: 847 kB
ℹ️ View Unchanged
|
Co-authored-by: Matthew <matthewcurtis@khanacademy.org>
| @@ -230,7 +230,7 @@ exports[`renderer rendering should render block math on mobile 1`] = ` | |||
| style="overflow-x: auto; overflow-y: hidden; margin-top: -10px; margin-bottom: -10px; transform: translate3d(0,0,0); padding: 10px 16px 10px 16px;" | |||
| > | |||
| <span | |||
| style="display: block; width: 100%; transform: scale(1, 1) translate(0, 8px); transform-origin: 0 0; opacity: 0;" | |||
| style="display: block; width: 100%; transform: scale(1, 1); transform-origin: 0 0; opacity: 1;" | |||
There was a problem hiding this comment.
As long as we're changing this line, could we get rid the transform and transform-origin properties? Do they have any effect now?
There was a problem hiding this comment.
They are still used- in two places. Non math content uses it and also it's used for the zoom-in, zoom-out effect.
| const shouldSlide = !disableEntranceAnimation && !visible; | ||
| const translateOffset = shouldSlide ? " translate(0, 8px)" : ""; | ||
| const opacity = shouldSlide ? 0 : 1; | ||
|
|
There was a problem hiding this comment.
suggestion: I find this (and lines 272-275) a little hard to follow with all the boolean logic and ternaries. It might be nice to pull this code out into a function that could be tested. IMO everything between lines 252 and 275 could go into that function. Calling it would look something like this:
const transitionStyle = computeTransitionStyle({
visible,
zoomed,
scale,
animateHeight,
disableEntranceAnimation,
});
I think I'd find reading the tests for that function easier than trying to work through all the possibilities in my head.
| disableEntranceAnimation: { | ||
| control: {type: "boolean"}, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Nice! TIL how to add controls to storybook.
Summary:
Having math animate with mathjax seems to be causing extra flickering. The math is fine after is "lands" and is holding still. This change adds a flag to the zoomable component so that we can disable animations.
zoomable-fix-demo.mov
Issue: https://khanacademy.atlassian.net/browse/LC-933
Test plan:
(Or check out the screencast)