MotiPressable: Changed containerStyle to style on parent element

[agrawal-rohit]

Description

Using the containerStyle prop on the AnimatedTouchable leads to elevation-based shadow not working on Android devices. However, they seem to work when using the style prop instead. This change is to meant to fix this.

Fixes: #149

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/fernandorojo/moti/8dor1CZaRzo8xmdpD5QEMpjgqeQZ
✅ Preview: https://moti-git-fork-agrawal-rohit-master-fernandorojo.vercel.app

[agrawal-rohit]

@nandorojo Could you review this PR when you get some time? Thanks

[nandorojo]

Sorry I wrote up a whole response but it looks like it didn't send from my phone. It's a bummer because I explained my reasoning here.

It's better to have these two components separate. A component shouldn't get styled based on its own hover state. If it does, then you can have unintended side effects.

For example, if a button gets hovered, and scales down to 0.5 when hovered, and the mouse is on the outer edge, then there will be a flicker. Hovering it makes it shrink, so once shrinks, it leaves the bounds of getting hovered.

See this video: https://www.loom.com/share/fd413dc7a014403c9cf44680218ff830

On the other hand, if you have a separate parent element which doesn't get affected by press/hover styles, then the interactable area is stable. Pressing/hovering a component doesn't affect its interactable area.

In this example, I'm tracking hover state in the wrapping div, solving this problem: https://www.loom.com/share/cbbefe1b7e7f4efd8ff641700df89e08

It is intentional to keep these components separate, so I don't think the intent of merging it into one makes the most sense for MotiPressable.

I should probably document this behavior more in the docs.

[nandorojo]

As for the containerStyle issue, I wonder if this is a problem with the underlying touchable from GestureHandler on Android.

[nandorojo]

Oh sorry I think I was referring to #151. Okay this I think should be fine. Are there any implications for iOS? Does maintain the same behavior on both?

[agrawal-rohit]

Sorry I wrote up a whole response but it looks like it didn't send from my phone. It's a bummer because I explained my reasoning here.

It's better to have these two components separate. A component shouldn't get styled based on its own hover state. If it does, then you can have unintended side effects.

For example, if a button gets hovered, and scales down to 0.5 when hovered, and the mouse is on the outer edge, then there will be a flicker. Hovering it makes it shrink, so once shrinks, it leaves the bounds of getting hovered.

See this video: https://www.loom.com/share/fd413dc7a014403c9cf44680218ff830

On the other hand, if you have a separate parent element which doesn't get affected by press/hover styles, then the interactable area is stable. Pressing/hovering a component doesn't affect its interactable area.

In this example, I'm tracking hover state in the wrapping div, solving this problem: https://www.loom.com/share/cbbefe1b7e7f4efd8ff641700df89e08

It is intentional to keep these components separate, so I don't think the intent of merging it into one makes the most sense for MotiPressable.

I should probably document this behavior more in the docs.

Ahh, I get the confusion. Your response did come through in the PR related to that (but this is a fresh PR). This PR only deals with the boxShadow issue still keeping the components separate (which makes a lot of sense after your reasoning).

[nandorojo]

Yeah looks like I did get that message on the other PR haha. A lot of open source PRs to review so it's hard to keep track sometimes. As long as this behavior is correct on iOS and Android, it should be fine to merge. Did you test on both? Could you add some before/after screenshots? Thanks!

[agrawal-rohit]

Oh sorry I think I was referring to #151. Okay this I think should be fine. Are there any implications for iOS? Does maintain the same behavior on both?

Yeah, the styling on iOS is left unaffected and the android issue styling gets fixed as per my testing

[agrawal-rohit]

Yeah looks like I did get that message on the other PR haha. A lot of open source PRs to review so it's hard to keep track sometimes. As long as this behavior is correct on iOS and Android, it should be fine to merge. Did you test on both? Could you add some before/after screenshots? Thanks!

Yeah, absolutely! I'll add a couple of screenshots here in another few minutes

[agrawal-rohit]

Hope this helps :)

Before

Code

import { GestureHandlerRootView, TouchableWithoutFeedback } from "react-native-gesture-handler";
import { View, Text } from "moti";

const App = () => {
  const AnimatedTouchable = Animated.createAnimatedComponent(
    TouchableWithoutFeedback
  );

  return (
    <GestureHandlerRootView style={{ flex: 1, padding: 50 }}>
      <AnimatedTouchable
        containerStyle={{
          padding: 20,
          overflow: "visible",
          backgroundColor: "white",
          shadowColor: "#1A2138",
          shadowOffset: {
            width: 0,
            height: 28,
          },
          shadowOpacity: 0.16,
          shadowRadius: 28,
          elevation: 28,
        }}
      >
        <View>
          <Text>Hello</Text>
        </View>
      </AnimatedTouchable>
    </GestureHandlerRootView>
  );
};

Behaviour

iOS

Android

After

Code

import { GestureHandlerRootView, TouchableWithoutFeedback } from "react-native-gesture-handler";
import { View, Text } from "moti";

const App = () => {
  const AnimatedTouchable = Animated.createAnimatedComponent(
    TouchableWithoutFeedback
  );

  return (
    <GestureHandlerRootView style={{ flex: 1, padding: 50 }}>
      <AnimatedTouchable
        style={{
          padding: 20,
          overflow: "visible",
          backgroundColor: "white",
          shadowColor: "#1A2138",
          shadowOffset: {
            width: 0,
            height: 28,
          },
          shadowOpacity: 0.16,
          shadowRadius: 28,
          elevation: 28,
        }}
      >
        <View>
          <Text>Hello</Text>
        </View>
      </AnimatedTouchable>
    </GestureHandlerRootView>
  );
};

Behaviour

iOS

Android

[nandorojo]

Thanks for investigating, and congrats on your first moti PR 🐼🎉

[agrawal-rohit]

Sweet! Thanks for creating this awesome package again 😇

[nandorojo]

Looks like this introduced regressions on iOS. I think I'm going to revert the style back. If you need to style the container on Android, I recommend using a parent view. However, containerStyle is no longer applying container styles (such as flex styles) which is important for absolute/flex positioning, as the name implies. You should be able to use the style prop just fine.

[clayrisser]

@nandorojo I understand your reasoning for the following comment, but there are definitely situations where it's ideal that the outer/parent component is the MotiView.

#152 (comment)

For example, if I try to create a circle with a border, and I want to animate the background color, you can see it completely ignores the border radius.

It should behave like the following.

But instead it behaves like this.

[clayrisser]

I'm wondering if there should be a prop that enables the user of the component to decide which is the inner and outer component.

[clayrisser]

While I could put the border styles in the from prop, that doesn't work for everything. For example, shadow styles won't work in the from prop, so an animated circle with a shadow is forced to look like the following.

[clayrisser]

Instead of a prop that lets us choose the inner and outer component, it would probably be better to have a prop that lets us have a single component instead of a wrapped component.

[nandorojo]

Can you create a snack of your example of expected vs. actual behavior? It might be easier to help there, since it isn't clear to me from these why it can't be achieved with the current structure.

[clayrisser]

I've figured out how to use this the way you designed it. I think the way you have it is fine.