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: Button and IconButton TouchableRipple borderRadius #4278

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jahirfiquitiva
Copy link

@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
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
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
Author

@lukewalczak would you mind taking another look, please?

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.

None yet

3 participants