[Box] Add support for boxes with semicircular edges #145
Conversation
nononoah
force-pushed
the
noahb/box-rounded-corners
branch
from
c43a8e4
to
e06f649
Compare
nononoah
force-pushed
the
noahb/box-rounded-corners
branch
from
ff0ed20
to
c6eba1d
Compare
| @@ -102,6 +102,7 @@ extension Box { | |||
|
|
|||
| public enum CornerStyle { | |||
| case square | |||
| case semicircular | |||
There was a problem hiding this comment.
I wonder if “pill” might be clearer? No preference, but in my head that makes me more immediately realize what it does.
There was a problem hiding this comment.
I'm open to changing it of course, but biased towards semicircular because
- it fits grammatically with the other cases (as an adjective)
- it is a generally understood geometric term (like square)
In my mind, pill is jargon and no different than chip, what Google's Material calls it. If you're familiar with the argot, it's familiar. Since most people working with Blueprint share this familiarity, I think it's a fine choice.
There was a problem hiding this comment.
Way overthought bikeshed:
I think part of the issue here is that the existing cases are purely the corner style where this new case is wanting to describe a shape as a whole (that is, affecting more than each corner individually). And I think we're left trying avoid using a “shape” word to describe a “corner.”
And my mind still reads it as a shape and expects a half a circle that's flat on the other side (that is to say, a literal semicircle).
FWIW SwiftUI avoids this issue by having this as a shape type (called Capsule) rather than a corner style.
I could see a few options:
- Rename this type/property to be “Shape” and then something like “Capsule” or “Pill” seems more right to me
- Put this into its own element type altogether (again, as Pill or Capsule IMO)
- Land as-is with the same or a different name (in which case I'd probably still lean against “semicircle” and just accept conflating shape and corner a bit)
(And Google's wrong, “chip” is a weird name)
There was a problem hiding this comment.
capsule is also a good name for this property
I wouldn't make this a different element since we'd have to duplicate all the shadow and border style stuff.
There was a problem hiding this comment.
Given the enthusiasm for capsule , I updated it. Still, I think it stands in contrast to its sibling cases.
And my mind still reads it as a shape and expects a half a circle that's flat on the other side (that is to say, a literal semicircle).
That's fair, though this is one impression I tried to avoid when renaming it from semicircle to semicircular. The latter may be ascribed to an object forming a semicircle, which is a fair attribution to such corners. This is similar if not identical semantically to square, I'd argue.
There was a problem hiding this comment.
Yeah, we've definitely got a less than idea parallelism here. I think were this has landed is more intuitive, though less … precise.
|
Pending additional review, this is ready to be merged. If someone with write access would like to bring it home, I'd appreciate that. |
| switch self { | ||
| case .square: | ||
| return 0 | ||
| case .semicircular: | ||
| return bounds.height / 2.0 |
There was a problem hiding this comment.
I think we should use the min of height and width so it works for vertical rects too
There was a problem hiding this comment.
Good call. Updated and added a snapshot test for it.
| @@ -18,13 +18,31 @@ class BoxTests: XCTestCase { | |||
| identifier: "clear") | |||
| } | |||
|
|
|||
| func test_cornerRadius() { | |||
There was a problem hiding this comment.
Could you delete the old image please?
There was a problem hiding this comment.
I think you mean this file, which has been deleted, though it's a bit tricky to tell from Github.
Please let me know if you're referring to another one.
There was a problem hiding this comment.
Oh oops, yeah sorry. GitHub on my phone really didn't want me to see it. Thanks!
| @@ -102,6 +102,7 @@ extension Box { | |||
|
|
|||
| public enum CornerStyle { | |||
| case square | |||
| case semicircular | |||
There was a problem hiding this comment.
Yeah, we've definitely got a less than idea parallelism here. I think were this has landed is more intuitive, though less … precise.
Summary
This PR adds the
capsulecase toBox.CornerStyle. This case sugars the following patterninto
Given the effect of
GeometryReader'sclosure on rightward drift and capture semantics and the relative ubiquity of this design element, I believe that this sugar is merited. I would further argue that this change comes at a small cost: a slight expansion of API surface area, and almost no implementation disruption.Changes
var radius: CGFloaton Box.CornerStyle tofunc radius(for:) -> CGFloatAdditional comments
This is my first PR, so I'm likely to have made a few process mistakes. Please correct me where I've gone wrong.