Skip to content
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

feat: getFrameCount #2568

Merged

Conversation

terrysahaidak
Copy link
Contributor

This PR implements a new method on AnimatedImage - getFrameCount. It's useful when you want to check how many frames in a gif. Sometimes gifs have only a single frame and you'd want to skip the animation and display just it.

Also, this PR fixes a typo - animatedImage.__typename__ was returning "\"AnimatedImage\"".

@terrysahaidak
Copy link
Contributor Author

just signed the CLA

@@ -15,6 +15,7 @@ describe("Animated Images", () => {
if (!animatedImage) {
return false;
}
animatedImage.getFrameCount();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! maybe here instead of returning true you can return the frame count and then test its value in expect. I guess in this particular test it will be one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you know exactly how many frames in this testing gif perhaps?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no but since it is test against both RN Skia and CanvasKit, if you put the result you get and all test pass it will be the correct result.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so it has a single frame. added test for it

@wcandillon
Copy link
Contributor

nice looks good

@terrysahaidak
Copy link
Contributor Author

terrysahaidak commented Aug 6, 2024

The CI failed with an error: No space left on device - seems not relevant

@wcandillon wcandillon merged commit 682b32a into Shopify:main Aug 7, 2024
10 of 11 checks passed
@wcandillon
Copy link
Contributor

thanks a lot :)

Copy link
Contributor

🎉 This issue has been resolved in version 1.3.11 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants