Conversation
…nctionality for image gifs.
…handle gif frame parsing.
…ack to this later.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
@claude review once |
…nd easier to read.
… to the purpose of the property.
…canvas approach comes from.
benchristel
left a comment
benchristel
left a comment
There was a problem hiding this comment.
This is looking good! Thanks for adding gifuct-js. I will come back and do a more thorough review later today — for now, I left some thoughts inline.
| }; | ||
| // Only re-run when src changes. | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [src]); |
There was a problem hiding this comment.
We don't list play, pause, and renderFrame as deps here — should we?
There was a problem hiding this comment.
I removed those dependencies because we really should only run this useEffect (decode all the GIF frames) on changes to src, if I added those other dependencies it would run this useEffect more often then necessary, and decoding the frames is the more expensive aspect of this code.
I can add them back, but I recommend against it.
There was a problem hiding this comment.
Seems like decoding should be done in a separate hook then; perhaps a usePromise hook similar to this one: https://www.npmjs.com/package/react-use-promise
const [frames, error] = usePromise(() => {
return decodeGifFrames(src)
}, [src]);
React.useEffect(() => {
if (frames != null) {
play();
}
return pause;
}, [play, pause, frames])(not suggesting we use that library; we could write our own)
There was a problem hiding this comment.
This seems like it could cause a significant re-write. Might I suggest a separate ticket to circle back to that? I would like to avoid another re-write at least in this PR. Thoughts?
| }; | ||
| // Only re-run when src changes. | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [src]); |
There was a problem hiding this comment.
Seems like decoding should be done in a separate hook then; perhaps a usePromise hook similar to this one: https://www.npmjs.com/package/react-use-promise
const [frames, error] = usePromise(() => {
return decodeGifFrames(src)
}, [src]);
React.useEffect(() => {
if (frames != null) {
play();
}
return pause;
}, [play, pause, frames])(not suggesting we use that library; we could write our own)
… down on animation drifting.
nishasy
left a comment
nishasy
left a comment
There was a problem hiding this comment.
This works really well!!
Just left one comment to make sure we have a story in the case where a gif is uploaded without a size, but that's not a blocker.
| src: string; | ||
| alt: string; | ||
| width?: number; | ||
| height?: number; |
There was a problem hiding this comment.
Could I request a new story for a GIF image that's saved with no size? The optional width and height here reminded me that's a possible case.
4 participants
Summary:
Functionality to pause and play image gifs. This includes:
Before:
Screen.Recording.2026-03-18.at.4.40.45.PM.mov
After:
Screen.Recording.2026-03-31.at.3.21.47.PM.mov
Issue: LEMS-3735
Test plan:
Highly recommend checking these Storybook pages to confirm the implementation:
/?path=/story/widgets-image-widget-demo--gif-image/?path=/story/editors-editorpage--with-all-flagsHere are some test gifs to use:
Also confirm that this experience is not visible when flags are off:
/?path=/story/editors-editorpage--demo