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

Added border curve style prop ("Squircle" effect - iOS only) #33783

Closed
wants to merge 8 commits into from

Conversation

eric-edouard
Copy link
Contributor

@eric-edouard eric-edouard commented May 7, 2022

Summary

NOTE: PR is based on #32017 which went stale for quite a long time but can now safely be closed

Since iOS 13+, it is possible to change the corner curve property on iOS in order to smoothen border radius and make it more "rounded" (also called "squircle")
Here's an article explaining in details what it is.
This property is also built in figma, but currently there is no way to implement this directly with react-native despite it being available natively on iOS.

Many open source react-native libraries were created in order to simulate this behaviour:
react-native-super-ellipse-mask
react-native-squircle-view
react-native-figma-squircle

But they rely on creating an SVG shape with the smoothed corners and masking the view behind. This makes it not very performant (flickering on mounting was a common side-effect)

This PR aims at implementing the property natively.

PR for the docs update: facebook/react-native-website#2785

Changelog

[iOS] [Added] - Added borderCurve style prop for smooth border radius (squircle effect)

Test Plan

We used the RNTester app and added an example with cornerCurve set to 'continuous' (only on iOS).

As the difference is quite subtle, we also made some more tests to better illustrate the difference (these are not in the RN-tester app):

IMG_0810

We overlapped two views with position: absolute, the one in the background has a red background and has cornerRadius set to false, and the one in the foreground is set to true. We can clearly see where the borders differs on the corners.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 7, 2022
@react-native-bot react-native-bot added Platform: iOS iOS applications. Type: Enhancement A new feature or enhancement of an existing feature. labels May 7, 2022
@analysis-bot
Copy link

analysis-bot commented May 7, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,821,839 +1,558
android hermes armeabi-v7a 7,213,962 +630
android hermes x86 8,133,894 +1,135
android hermes x86_64 8,111,836 +963
android jsc arm64-v8a 9,700,247 +1,631
android jsc armeabi-v7a 8,454,450 +692
android jsc x86 9,650,465 +1,203
android jsc x86_64 10,248,242 +1,022

Base commit: f3db6cc
Branch: main

Copy link
Contributor

@janicduplessis janicduplessis left a comment

Choose a reason for hiding this comment

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

Nice! PR looks good to me.

Sorry this is taking forever to merge

cc @cortinico

@janicduplessis
Copy link
Contributor

@eric-edouard Did you test if this work with the Fabric renderer? I think RN tester uses it by default now.

Can you also rebase, I think CI was broken recently and was fixed here 94341f0.

@janicduplessis
Copy link
Contributor

@eric-edouard
Copy link
Contributor Author

@janicduplessis you're correct, this doesn't work with Fabric yet. Will take a look into it! Thanks

@eric-edouard eric-edouard marked this pull request as draft May 9, 2022 16:28
@eric-edouard eric-edouard marked this pull request as ready for review May 14, 2022 14:06
@eric-edouard
Copy link
Contributor Author

@janicduplessis I've adapted the logic to have it work with Fabric, works well on my end with the same testing strategy as in the PR description.
I mostly duplicated the logic found in both BorderStyle (It being a string prop and an enum in native) and BorderRadius (as it affects corners)
it did seem like a lot to add in many places for such a small feature 😄 but couldn't see how to make it simpler.

@analysis-bot
Copy link

analysis-bot commented May 14, 2022

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: f3db6cc
Branch: main

@eric-edouard
Copy link
Contributor Author

@janicduplessis quick bump 🙂

@cipolleschi
Copy link
Contributor

Cold you rebase this? I can see test_windows fail, but they should not be affected by your PR, and the CI is green on main. After the rebase, I'll import it!

@eric-edouard
Copy link
Contributor Author

@cipolleschi rebased + tested again on RNTester app after merge, all good

@eric-edouard
Copy link
Contributor Author

@cipolleschi quick bump!

@facebook-github-bot
Copy link
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
Contributor

@cipolleschi cipolleschi left a comment

Choose a reason for hiding this comment

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

Sorry if this took a while to be imported. Thanks again for the contribution.
Can I ask if you can rebase it and fix the small comment I left?

Besides these, it looks very good.

React/Views/RCTBorderCurve.h Outdated Show resolved Hide resolved
@eric-edouard
Copy link
Contributor Author

eric-edouard commented Jul 17, 2022

@cipolleschi No worries! Fixed the new line and aligned with main. Let me know if there's anything else

@facebook-github-bot
Copy link
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @eric-edouard in 8993ffc.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Jul 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: iOS iOS applications. Type: Enhancement A new feature or enhancement of an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants