Skip to content
This repository was archived by the owner on Nov 3, 2025. It is now read-only.

Conversation

@ppamorim
Copy link

@ppamorim ppamorim commented Dec 16, 2020

[Draft] Waiting for 2021

Summary

This PR replaces the use of masks to create rounded corners. Instead, it uses view.layer.maskedCorners and view.layer.cornerCurve where iOS will only mask the corners of the view. This solution prevents offscreen rendering, this is a problem when applied against complex lists.

Side Effects

Indeed these changes will make the component look odd on iOS 12 and below. But there are compelling reasons described below (explanation has to be improved).

Reason

I noticed a slow performance of this library when applied against a complex list. Since this is just a draft, I will improve and add examples of how the performance has been improved.

Here is the example before and after:

Note that Apple does the same with their UISegmentedControl and while the `UIViewController transition in devices with rounded borders in the screen.

I can see a massive improvement in complex subviews (i.e. UITableView). The component used to be choppy, probably around 30 fps with intermittent frames locked. With this new version it's stuck at 60 fps and runs smooth.

Requirements (place an x in each [ ])

  • I've read and understood the Contributing Guidelines and have done my best effort to follow them.

  • I've read and agree to the Code of Conduct.

  • I've written tests to cover the new code and functionality included in this PR.

@CLAassistant
Copy link

CLAassistant commented Dec 16, 2020

CLA assistant check
All committers have signed the CLA.

view.isOpaque = alpha == 1.0
view.layer.cornerRadius = Constants.dragIndicatorSize.height / 2.0
if #available(iOS 13.0, *) {
view.layer.cornerCurve = .continuous

Choose a reason for hiding this comment

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

Since this is a pill shape, I think we want the standard circular corners instead of continuous

Copy link
Author

Choose a reason for hiding this comment

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

Does it make sense? Both are visually similar but the continuous one does the same as the current mask solution. Did you test my code?

Choose a reason for hiding this comment

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

I think you're conflating switching to cornerRadius (which the dragIndicatorView already does) and specifying a continuous vs circular curve. A continuous curve is nice aesthetically when a corner is being rounded by itself. It creates a subtle "squircle" shape, rather than a simple circular radius that has a jarring transition to the flat part of the shape. This works great for something like the sheet view itself, so I support using continuous in that code.

However, in this drag view the two corners meet, and when you use the continuous style, this leads to point edges. The circular style has the desired effect, since it's two quarter circles that match perfectly. Here's a blown-up demonstration of the two effects when the radius is equal to the height / 2:

Simulator Screen Shot - iPhone 12 Pro Max - 2021-01-29 at 12 12 18

I hope that make sense!

Copy link
Author

@ppamorim ppamorim Feb 2, 2021

Choose a reason for hiding this comment

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

I am pretty sure that the continous solution is still idea for this project. The roundness of the curve is not the main point and the current solution already uses the continous solution. The rounded UIBezierPath does this automatically.

@ppamorim ppamorim marked this pull request as ready for review May 7, 2021 11:13
@ppamorim
Copy link
Author

ppamorim commented May 7, 2021

I have been using this fix in production since the first version and it seems to be working fine. I am opening the PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants