-
Notifications
You must be signed in to change notification settings - Fork 110
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
Lottie icons 2408 (Milestone 2) #2462
base: main
Are you sure you want to change the base?
Lottie icons 2408 (Milestone 2) #2462
Conversation
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 animations looks great, and the behavior when clicking multiple times is perfect.
1. Only animate inactive → active
Note that for commands that have an active/inactive state (pin, pinChildren, table view, bold, italic, etc) the animation should only occur when going from inactive → active. It should not be animated from active → inactive, but rather just go gray.
For shortcuts that do not define an isActive
function, then animation should be performed whenever the button is clicked.
2. Preserve click area
You may already be aware, but the click area of the toolbar icons has gotten smaller. In particular the extra padding below.
Current Behavior
Expected Behavior
main
src/@types/Icon.ts
Outdated
} | ||
|
||
export default Icon | ||
export interface AnimatedIconType { |
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 think you can use interface AnimatedIconType extends IconType
here for inheritance.
By convention on this project we generally export a single, default export per file, with the filename matching the type name. So IconType
would go in IconType.ts
and AnimatedIconType
would go in AnimatedIconType.ts
.
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.
Good idea to use interface AnimatedIconType extending IconType.
Also, I will separate the types into their own files following the project convention.
const color = style.fill || fill || token('colors.fg') | ||
|
||
// Local state to manage animation status | ||
const [isAnimated, setIsAnimated] = useState(animated) |
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.
Maybe you're setting up for future functionality, but is there a reason you are creating local state instead of just using the animated
prop?
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.
You're right that we could directly use the animated prop for the current functionality. However, the reason I’ve set up local state (isAnimated) is to allow for more flexible control over the animation in future updates.
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.
Unless you plan on using it within the same PR, I think I'd prefer we remove it. We don't want to add additional complexity unless there is a real need.
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.
That makes sense. If more control over the animation becomes necessary in future updates, we can always revisit it and reintroduce the state as needed.
/** Calls the animationComplete callback if provided. */ | ||
const handleAnimationComplete = () => { | ||
if (animationComplete) { | ||
animationComplete() | ||
} | ||
} |
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.
Can you pass animationComplete
directly to <LottieAnimation>
instead of having the wrapper?
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.
That’s a good point; we can pass animationComplete directly to for now. I initially set it up with a wrapper to accommodate future enhancements, like ensuring a smooth reappearance of the animation.
e185afd
to
a3df834
Compare
Regarding the 'Preserve click area' point, it seems to be related to the scaling adjustments planned for milestone 3. |
…Type.ts, update imports.
…te only transitions from inactive to active.
@@ -85,6 +85,7 @@ | |||
"@mui/material": "^6.1.0", | |||
"algoliasearch": "^5.2.4", | |||
"axios": "^1.7.7", | |||
"canvas": "^2.11.2", |
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.
Is canvas
a new peer dependency requirement? I'm not seeing where this used.
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 canvas package is essential for resolving HTMLCanvasElement.prototype.getContext errors in our test environment. Without this package, running yarn test would result in numerous errors, as the canvas-related functionalities required for simulations would be missing.
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.
Okay. It's not listed as a peer dependency of lottie-react
, although I do see jest-canvas-mock
. Is the canvas
package related to that? Just trying to understand what depends on this new package, as it was not needed before.
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 react-lottie library, which is used for rendering Lottie animations in React, does rely on the canvas element through the bodymovin library. Bodymovin (part of lottie-web) uses canvas or SVG as its renderer, depending on the animation's complexity and settings. When adding react-lottie, this canvas dependency can require additional setup in testing environments like Jest or Vitest, particularly for mock or DOM rendering support.
@ohrytskov -- There's still just one failing test case: https://github.com/cybersemics/em/actions/runs/11522295561/job/32077852690?pr=2462 When this is ready for review, please just let me know and I'll be happy to take a look right away. Thanks! |
@trevinhofmann, all checks have passed. |
…om new animation assets.
…lements in Puppeteer tests.
#2408