Skip to content

Fix onPress handler mapping on web elements#1187

Merged
msand merged 1 commit intosoftware-mansion:developfrom
rodolfovilaca:fix-onpress-web-elements
Dec 9, 2019
Merged

Fix onPress handler mapping on web elements#1187
msand merged 1 commit intosoftware-mansion:developfrom
rodolfovilaca:fix-onpress-web-elements

Conversation

@rodolfovilaca
Copy link
Copy Markdown

Summary

So onPress handlers are not working on web elements like <Path />. I've noticed that because I am creating a graph in react-native-svg that works on all platforms and I needed the onPress functionality on a Path element. I've searched for some issues and saw that others are suffering the same problem:

I think that this problems are related to this line that validates if the prop name is a valid prop for touchables on the web so what I did (to be as simple as possible) is to map the onPress handler to onClick handler on the web. Only impacts the web implementation.

obs: I don't actually know if the current behaviour is the desired one and if this is
actually a problem that should be fixed, or we won't have touchables events on path
elements.

Test Plan

What's required for testing (prerequisites)?

No prerequisites.

What are the steps to reproduce (after prerequisites)?

Draw a simple <Path /> element and add some onPress handler on the web.

Compatibility

OS Implemented
Web
Android N/A
IOS N/A

Checklist

  • I have tested this on a device and a simulator
  • I added the documentation in README.md (N/A)
  • I mentioned this change in CHANGELOG.md
  • I updated the typed files (TS and Flow)(N/A)
  • I added a sample use of the API in the example project (example/App.js) (N/A)

@msand
Copy link
Copy Markdown
Contributor

msand commented Nov 11, 2019

Great, thanks for this. You can remove the note from the changelog, it's created automatically, I should update the pr template not to include it in instructions. Additionally, could you check that there's no onClick property from before? In case someone has different event handlers for web and native, then it would be better not to override the existing web handler.

@rodolfovilaca rodolfovilaca force-pushed the fix-onpress-web-elements branch from 50ed4a5 to 704094c Compare November 12, 2019 00:12
@rodolfovilaca
Copy link
Copy Markdown
Author

For sure, I made the changes you requested. Should we try to map other web events? onPressIn and onPressOut like in the docs? I mean we could possibly map them to other web events(onTouchEnd, onTouchStart), but maybe we should discuss them idk...

@msand
Copy link
Copy Markdown
Contributor

msand commented Dec 9, 2019

Looks good for now, can make another pr if more handlers are needed. Thanks!

@msand msand changed the base branch from master to develop December 9, 2019 19:16
@msand msand merged commit 151da0c into software-mansion:develop Dec 9, 2019
@msand
Copy link
Copy Markdown
Contributor

msand commented Dec 9, 2019

🎉 This PR is included in version 9.13.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

@msand msand added the released label Dec 9, 2019
@mikeaustin
Copy link
Copy Markdown

View has onLayout, but it looks like SVG components do not? Where should I look for adding that support?

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