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

Add gif and webp support to Fresco #8455

Closed
wants to merge 1 commit into from

Conversation

charpeni
Copy link
Contributor

@charpeni charpeni commented Jun 27, 2016

Motivation #7760

Test plan
Build from source with a gif image, everything works fine.

However, i'm not sure about the BUCK part. While building I saw the dependencies I added being downloaded, but the app built with BUCK doesn't support gif. Maybe someone with more experience with BUCK than me can check this?

@ghost
Copy link

ghost commented Jun 27, 2016

By analyzing the blame information on this pull request, we identified @cjhopman and @mkonicek to be potential reviewers.

@ghost ghost added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jun 27, 2016
@ide
Copy link
Contributor

ide commented Jun 28, 2016

@andreicoman11 do you guys want this on facebook's side?

@andreicoman11
Copy link
Contributor

I think we want this.
@bestander these libraries don't automatically get built as part of the app, unless you explicitly include each library, right?

@bestander
Copy link
Contributor

@andreicoman11, regarding app sizes.

  • Gradle will include these libraries by default because they are on "compile" target now. Prod apps would want to exclude them with proguard or some custom gradle scripts.
  • Buck will include everything that is required via dependency tree, i.e. if there are modules that use imagepipeline target those will be downloaded and included

The short answer to all is yes, they will automatically get built as part of the app

@andreicoman11
Copy link
Contributor

That is bad news. Afaik, these libraries are quite big, it sounds like we shouldn't include them automatically for everybody.

@charpeni
Copy link
Contributor Author

@andreicoman11, before version 0.10 of Fresco these libraries were included by default into the Fresco library. Since 0.10, these libraries has been split up, so in theory we don't add much weight to React Native since it was there previously. But I understand what you mean, I just want to be sure you know all facts.

If you still don't want it into React Native, I will add some documentation about gif and webp support into the Image component, just let me know.

@andreicoman11
Copy link
Contributor

Thanks for the info @charpeni . The fresco split up was requested by multiple parties, since the whole library was so big. I think we should keep the core size as small as we can, and in this case, the majority of the people won't be needing those libraries.
If you could add docs about gif and webp support, that would be much appreciated.

@charpeni
Copy link
Contributor Author

Good, I will do that.

@charpeni charpeni closed this Jun 29, 2016
ide referenced this pull request Jul 3, 2016
Summary: This adds support for animated GIFs on Android! Looking forward to some meme apps :)
Also, Fresco is awesome.

Closes #2997.

@​public

Reviewed By: @mkonicek

Differential Revision: D2477540

committer: Service User <svcscm@fb.com>
ghost pushed a commit that referenced this pull request Jul 31, 2016
Summary:
Motivation #8455
Fixes #8501

With a bonus fix typo !

![screen shot 2016-07-25 at 14 16 01](https://cloud.githubusercontent.com/assets/7189823/17112118/9f06fe04-5272-11e6-83e9-ddf11573aa5e.png)
Closes #8951

Differential Revision: D3647816

Pulled By: mkonicek

fbshipit-source-id: e0349275045cae2922b4bb43bcb99af4c6ef1170
bartolkaruza pushed a commit to immidi/react-native that referenced this pull request Aug 3, 2016
Summary:
Motivation facebook#8455
Fixes facebook#8501

With a bonus fix typo !

![screen shot 2016-07-25 at 14 16 01](https://cloud.githubusercontent.com/assets/7189823/17112118/9f06fe04-5272-11e6-83e9-ddf11573aa5e.png)
Closes facebook#8951

Differential Revision: D3647816

Pulled By: mkonicek

fbshipit-source-id: e0349275045cae2922b4bb43bcb99af4c6ef1170
mpretty-cyro pushed a commit to HomePass/react-native that referenced this pull request Aug 25, 2016
Summary:
Motivation facebook#8455
Fixes facebook#8501

With a bonus fix typo !

![screen shot 2016-07-25 at 14 16 01](https://cloud.githubusercontent.com/assets/7189823/17112118/9f06fe04-5272-11e6-83e9-ddf11573aa5e.png)
Closes facebook#8951

Differential Revision: D3647816

Pulled By: mkonicek

fbshipit-source-id: e0349275045cae2922b4bb43bcb99af4c6ef1170
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants