wix / react-native-navigation

A complete native navigation solution for React Native
https://wix.github.io/react-native-navigation/
MIT License
13.04k stars 2.67k forks source link

FastImage support #6596

Closed mrousavy closed 4 years ago

mrousavy commented 4 years ago

Issue Description

Currently the library (mostly the animations, like Shared Element Transitions) assumes that all images are presented in a RCTImageView (see this line). Unfortunately the default react Image component doesn't quite provide the features I need, therefore I switched to the FastImage component from react-native-fast-image.

While animations work somewhat as expected (even resizeMode transitions correctly, so it looks pretty smooth), I noticed that the borderRadius (cornerRadius) animation doesn't work.

RCTImageView

ezgif com-video-to-gif-2

FFFastImageView

ezgif com-video-to-gif

Solution

I've tried to also return ViewTypeImage for Fast Images, like in this diff, but that didn't magically affect cornerRadius :(

FastImage "natively supports borderRadius", but I couldn't figure out how to apply a cornerRadius to a FFFastImageView (which btw is only a SDWebImage on iOS)

Environment

mrousavy commented 4 years ago

Also note that the parent's transforms aren't applied. E.g.: When the parent view scales down to 90% of it's size, the Shared Element Transition should begin at that smaller size instead of jumping back to the origin. @yogevbd fixed that, but apparently I can still reproduce it with FastImages:

ezgif com-optimize

yogevbd commented 4 years ago

The real problem is that fast-image wraps an Image view with another RCTView, the nativeID is passed to the Image but the styles are passed to the RCTView wrapper. We can open a PR for react-native-fast-image which will add the ability to pass styles to the image itself or we can hack this specific behaviour in RNN. https://github.com/DylanVann/react-native-fast-image/blob/a53f3ab8bd171739c00bbe4ef2c1e5aa8aa7ce41/src/index.tsx#L153

@mrousavy @guyca wdyt?

guyca commented 4 years ago

Nice work tracking down the root cause!! Introducing api changes to react-native-fast-image only to support SET isn't ideal. Ideally we want our users to design their screens as they wish and for SET to just work.

We can add dedicated animators for FastImage like you suggested. Isn't this the right thing to do regardless? as FastImage is a different type of image, it's only natural that it will require a different animator as its implementation is different.

mrousavy commented 4 years ago

I agree with @guyca for the SET goal, but I'd also say it would be better for the fast-image library to not render the image under a parent View (it does that to support borderRadius by applying it to the View layer and disabling overflow, which is a dirty hack and should be solved by allowing native borderRadius support for FastImage (SDWebImage and Glide), just like the RCTImage does.)

guyca commented 4 years ago

I thought Glide has an api to apply border radius on images 🤔 At any case adding border radius to an image is quite simple with an OutlineProvider.

Can't hurt to contact the maintainers and explore the possibilities of submitting a PR, but it might be the longer route as well..

mrousavy commented 4 years ago

It's definitely the longer route, as the maintainer rarely maintains the project. It's a solution for the long run, I'll maybe create a PR over there later which natively supports the borderRadius prop.

yogevbd commented 4 years ago

I actually have more robust solution, we can try to look for the cornerRadius property in the view superviews with the same frame. wdyt?

mrousavy commented 4 years ago

Would this also mean the parent's scale/transforms get applied? (e.g. FastImage position like in https://github.com/wix/react-native-navigation/issues/6596#issuecomment-694099788)

guyca commented 4 years ago

Sounds like a good enough solution! I would also suggest to keep all FastImage related logic in dedicated animators so we don't end up with animators with if-else statements that handle two different views.

mrousavy commented 4 years ago

Sounds like a good enough solution! I would also suggest to keep all FastImage related logic in dedicated animators so we don't end up with animators with if-else statements that handle two different views.

A polymorphic approach would probably be the best solution, similar to my proposed approach in https://github.com/wix/react-native-navigation/pull/6591. This opens up the possibility to extend the API at any point in the future (e.g. when a new image component gets released that supports looking into the 4th dimension's v̶̧͇̯̹͍̥̦̺̐̽̈́͐̈̏̀͊̓̈́̉̔͛͘͜͠o̶̬̞̜̹̥̯̗͗͌̀͗̂̔̃̃͒̓̈́̚̚̚͜͠í̴̖͇̲͔d̵̻͂͋͝)

yogevbd commented 4 years ago

@mrousavy I will do the same for the transform property.

mrousavy commented 4 years ago

Awesome, nice solution!