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

Fix tap target cut off on the top half #21

Merged
merged 2 commits into from
Nov 1, 2020

Conversation

Chriscbr
Copy link
Contributor

@Chriscbr Chriscbr commented Jan 7, 2020

Fixes issue #20

I believe the issue was with part of the specified touch target being covered by another element. I tried changing the z-index, but that didn't resolve the problem. In the end, changing the outer component to use padding instead of margin seems to fix the issue, and doesn't have any impact on the positions of any of the other components (as far as I can tell).

Aside: I'm a bit new to React Native, so when I tried building this project and fixing the bug, I encountered a couple of issues because I was doing this on Windows. The build initially failed because the android/app/build.gradle file tries to call a macOS specific command ('security') - so I had to remove those pieces to get the app working. The other issue was the pre-push git hook from Husky that runs eslint did not work because of something with formatting the folder's name in quotes - changing the command to eslint src/ --ext ... (without quotes around the folder name) seemed to fix it on Windows. I imagine these aren't as important issues to fix, but I just figured I'd mention them in case anyone else has the same issues. 👍

@mmazzarolo
Copy link
Owner

Hi @Chriscbr , thanks for submitting this PR!

I belive issue was with part of the specified touch target being covered by another element. I tried changing the z-index, but that didn't resolve the problem. In the end, changing the outer component to use padding instead of margin seems to fix the issue, and doesn't have any impact on the positions of any of the other components (as far as I can tell).

Nice! Have you/can you try it on iOS? Otherwise I can check it once I have some free time :)
I'm still not sure why the touchable area doesn't work correctly on Android 🤔

Just FYI, the Score component uses a Button component underneath, where I set a a bigger touchable area (aka HitSlop) just to avoid this issue... but I might have not tested it enough on Android 😓

side: I'm a bit new to React Native, so when I tried building this project and fixing the bug, I encountered a couple of issues because I was doing this on Windows. The build initially failed because the android/app/build.gradle file tries to call a macOS specific command ('security') - so I had to remove those pieces to get the app working. The other issue was the pre-push git hook from Husky that runs eslint did not work because of something with formatting the folder's name in quotes - changing the command to eslint src/ --ext ... (without quotes around the folder name) seemed to fix it on Windows. I imagine these aren't as important issues to fix, but I just figured I'd mention them in case anyone else has the same issues. 👍

Thanks for reporting! I never worked on RN on Windows, so yeah, it might have some bugs... We can add these infos in CONTRIBUTING.md 👍

@Chriscbr
Copy link
Contributor Author

Chriscbr commented Jan 8, 2020

@mmazzarolo Unfortunately I don't have the resources to run the app on iOS - so maybe you can test to see if this same issue occurs on Apple devices (and/or check if my proposed change doesn't affect the UI layout). I noticed the HitSlop attribute, but I guess it is not working as expected in this particular situation (or at least on Android). Let me know what you find out!

@mmazzarolo
Copy link
Owner

@Chriscbr took me only almost an year but LGTM, merging 😅

@mmazzarolo mmazzarolo merged commit c132bbf into mmazzarolo:master Nov 1, 2020
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.

2 participants