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: set default cursor for TouchableRipple to 'pointer' #2515

Merged
merged 1 commit into from
Jan 22, 2021

Conversation

RafikiTiki
Copy link
Member

Summary

While I was working on client's app code I've come to a realisation that in every place where Paper's <TouchableRipple /> component was used we needed to pass additional style to ensure pointer cursor on the web. After some digging I've found that it's caused because TouchabeRipple is using <TouchableWithoutFeedback /> under the hood and based on this: necolas/react-native-web#1847 (comment) that's exactly what's causing react-native-web to NOT "inject" cursor: 'pointer' style on the web.

Test plan

In the example app:

  • on mobile device/simulator everything should work exactly the same as before this change.

  • on the web <TouchableRipple /> component should have pointer cursor

@github-actions
Copy link

The mobile version of example app from this branch is ready! You can see it here

.

@@ -253,6 +254,7 @@ TouchableRipple.supported = true;
const styles = StyleSheet.create({
touchable: {
position: 'relative',
...(Platform.OS === 'web' && { cursor: 'pointer' }),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need this conditional? What's gonna happen if we pass cursor: 'pointer' in RN?

@mikehardy
Copy link
Contributor

Ironically, the one place I noticed this goes to 'pointer' from 'default' is in DataTable.Cell, where most of my data is just text (except a couple IconButtons) and they all have pointers making them look clickable so now I need to pass in cursor: default 😆

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

Successfully merging this pull request may close these issues.

3 participants