-
Notifications
You must be signed in to change notification settings - Fork 120
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
MotiPressable: Changed containerStyle to style on parent element #152
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/fernandorojo/moti/8dor1CZaRzo8xmdpD5QEMpjgqeQZ |
@nandorojo Could you review this PR when you get some time? Thanks |
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 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 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 I should probably document this behavior more in the docs. |
As for the |
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? |
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 |
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, the styling on iOS is left unaffected and the android issue styling gets fixed as per my testing |
Yeah, absolutely! I'll add a couple of screenshots here in another few minutes |
Hope this helps :) BeforeCodeimport { 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>
);
}; BehaviouriOSAndroidAfterCodeimport { 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>
);
}; BehaviouriOSAndroid |
Thanks for investigating, and congrats on your first |
Sweet! Thanks for creating this awesome package again 😇 |
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, |
@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. 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. |
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. |
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. |
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. |
I've figured out how to use this the way you designed it. I think the way you have it is fine. |
Description
Using the
containerStyle
prop on theAnimatedTouchable
leads to elevation-based shadow not working on Android devices. However, they seem to work when using thestyle
prop instead. This change is to meant to fix this.Fixes: #149