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: use Pressable in TouchableRipple #3400

Conversation

szymonrybczak
Copy link
Contributor

This PR replaces Touchables with Pressable within TouchableRipple.

Related issues:

Summary

Test plan

@callstack-bot
Copy link

callstack-bot commented Oct 5, 2022

Hey @szymonrybczak, thank you for your pull request 🤗. The documentation from this branch can be viewed here.

Copy link
Collaborator

@hurali97 hurali97 left a comment

Choose a reason for hiding this comment

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

Thanks for raising this PR, much appreciated 🌟.

There are a couple changes that can be addressed.

src/components/TouchableRipple/TouchableRipple.native.tsx Outdated Show resolved Hide resolved
src/components/TouchableRipple/TouchableRipple.native.tsx Outdated Show resolved Hide resolved
@hurali97
Copy link
Collaborator

hurali97 commented Oct 12, 2022

@szymonrybczak Additionally, there are some regressions that causes instability on web. I am attaching the screenshots of the relevant components that are affected by the PR changes. If you can take a look at these, it will be so helpful.

Platform(web):

  • The back icon button mis aligns on web.
Bottom-Navigation
  • layout of bottom-navigation shrinks on web.
Bottom-Navigation
Buttons
  • Ripple doesn't respect the border-radius.
Buttons
Icon-Buttons
  • Doesn't center the icon correctly, and ripple only happens on icon, not the whole component.
Icon-Buttons
Radio-Button-Group
  • Ripple is not round, as compared to the baseline.
Radio-Button-Group
SearchBar
  • Related to Icon Buttons.
SearchBar
Segmented-Buttons
  • Ripple doesn't respect the border-radius.
Segmented-Buttons
Text-Input
  • Related to Icon Buttons.
Text-Input
Toggle-Buttons
  • Ripple doesn't fill the container, it only fills the inner view.
Toggle-Buttons
Touchable-Ripple-Difference
Touchable Ripple New: Touchable-Ripple-New
Touchable Ripple Baseline: Touchable-Ripple-Old

@szymonrybczak
Copy link
Contributor Author

@hurali97 Yes, sure. This was really the problem, I fixed it! 😅

@szymonrybczak szymonrybczak requested review from hurali97 and removed request for lukewalczak October 12, 2022 14:31
@lukewalczak
Copy link
Member

@szymonrybczak thanks for adjusting the comments and fixing the issue! @hurali97 could you please give another round of testing and checking the PR? 👌🏽

@hurali97
Copy link
Collaborator

@szymonrybczak Thanks for incorporating them real quick. 🌟

@lukewalczak I'll re-run the tests and re-review the PR tomorrow morning.

Copy link
Member

@lukewalczak lukewalczak left a comment

Choose a reason for hiding this comment

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

Great work 🎉 Thanks @szymonrybczak for your effort and @hurali97 for reviewing and testing the PR!

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

Successfully merging this pull request may close these issues.

4 participants