Skip to content

fix: Button and IconButton TouchableRipple borderRadius #4278

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

Merged
merged 9 commits into from
Apr 22, 2025

Conversation

jahirfiquitiva
Copy link
Contributor

@jahirfiquitiva jahirfiquitiva commented Jan 16, 2024

Motivation

Resolve issue #4266

Description

Removes the borderRadius from the IconButton inner TouchableRipple.

Using overflow: hidden is enough for the inner views to fit the parent view, even taking care of the parent view borderRadius.

Before After
Shot 2024-01-15 at 20 26 40 Shot 2024-01-15 at 20 27 22

After the changes, we can even set custom border radius for each corner of IconButton and the TouchableRipple will fill the button properly

Shot.2024-01-15.at.20.28.01.mp4

Updates the Button inner TouchableRipple props to make the ripple fill the Button surface.

The overflow: hidden trick could not be applied to Button as its Surface can be elevated, and when using overflow: hidden the elevation shadow will not be displayed correctly.

It's slightly hard to notice, but before, in the rounded corners it would be a space of ~1px due to the borderWidth

Before After
Shot 2024-01-15 at 20 23 54 Shot 2024-01-15 at 20 22 40
Shot 2024-01-15 at 20 39 19@2x Shot 2024-01-15 at 20 38 37@2x

Adds contentStyle prop to IconButton, similar to the one in Button

Previously there was no way to customize the IconButton padding, as if we set it to the style prop, it would cause the inner TouchableRipple to not fill the IconButton. Now, contentStyle allows doing so by setting the styles to the inner TouchableRipple view.

Test plan

Run yarn example web to run the example project and view the updated examples for Button and IconButton

@callstack-bot
Copy link

callstack-bot commented Jan 16, 2024

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

| 'borderRadius'
>
>;
export const getButtonTouchableRippleStyle = (
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add a unit test covering that functionality?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lukewalczak for this specific function? or for the Button component? Because the previous tests were still passing, meaning the functionality did not affect their styles. The only test that changed was for the outlined button, where the result inner border radius is 19 (20 - 1 (border))

Copy link
Member

Choose a reason for hiding this comment

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

@jahirfiquitiva Perfectly for both cases

@lukewalczak lukewalczak changed the title Fix Button and IconButton TouchableRipple borderRadius fix: Button and IconButton TouchableRipple borderRadius Jan 22, 2024
@jahirfiquitiva
Copy link
Contributor Author

@lukewalczak I have added a test for an outlined button with custom radius. There were already tests for non-outlined buttons with custom radius

@jahirfiquitiva
Copy link
Contributor Author

@lukewalczak would you mind taking another look, please?

@lukewalczak lukewalczak self-assigned this Apr 17, 2025
@lukewalczak lukewalczak merged commit 7e938cf into callstack:main Apr 22, 2025
1 check passed
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