-
Notifications
You must be signed in to change notification settings - Fork 45
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
Introduce Decorate element #178
Conversation
8438ffa
to
56bd170
Compare
56bd170
to
d414727
Compare
d414727
to
255fce9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good for the most part, not sure about the Layout
question 🤔 we should add some snapshot tests too!
Is anyone using an element like this now? I think in most elements I've seen where the tap target is larger than its visible affordance, it lays out the background normally, and then adjusts the tap target in the backing control. This seems to invert that idea, making the layout size match the tap target, and then taking the background out of layout. I'd like to see how this is expected to be used. |
…rate-background * origin/kve/layout-writer: Introduce LayoutWriter, for easier custom layouts without needing to use the Layout protocol. Bumping versions to 0.18.0 Some changes to the ElementPreview type to use it within Market's preview element. Add `AccessibilityContainer.identifier`
Redoing this ontop of |
7490aae
to
370beda
Compare
370beda
to
1551bc6
Compare
* origin/main: Bump redcarpet from 3.5.0 to 3.5.1 Prep 0.19.1 Update CHANGELOG Revert "Merge pull request #190 from square/kve/validate-elements-are-structs" Prep 0.19.0 Remove Travis builds Set up Github Actions Add a test for how often the builder is called Code review, add a couple methods that I used in validating the behaviour of negative positions Validate elements are structs Fix empty environment referencing a forever-living MeasurementCache. Fix SPM build
This should be good for another review pass. |
public init( | ||
layering : Layering, | ||
position: Position, | ||
wrapping: () -> Element, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if it makes sense to take a closure here. Most Blueprint & SwiftUI containers that take a closure do it to support builders that accumulate multiple children (like stacks) or that have parameters (like GeometryReader). SwiftUI secondary views don't use builder closures either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the closure since it gives you an immediate scope to set things up, but I'll remove for consistency for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'll keep it on the Element
extension though since it makes the code read a bit more nicely, eg:
.thingy()
.decorate(layering: .below, position: .inset(5)) {
Box(backgroundColor: .init(white: 0.0, alpha: 0.1), cornerStyle: .rounded(radius: 17))
}
} | ||
|
||
/// What corner the decoration element should be positioned in. | ||
public enum Corner : Equatable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of Corner
& Position
, can you add an Alignment
type? I culled it from the alignment guides stuff because it was unused, but it looks like this:
public struct Alignment: Equatable {
static let center = Alignment(horizontal: .center, vertical: .center)
static let leading = Alignment(horizontal: .leading, vertical: .center)
static let trailing = Alignment(horizontal: .trailing, vertical: .center)
static let top = Alignment(horizontal: .center, vertical: .top)
static let bottom = Alignment(horizontal: .center, vertical: .bottom)
static let topLeading = Alignment(horizontal: .leading, vertical: .top)
static let topTrailing = Alignment(horizontal: .trailing, vertical: .top)
static let bottomLeading = Alignment(horizontal: .leading, vertical: .bottom)
static let bottomTrailing = Alignment(horizontal: .trailing, vertical: .bottom)
var horizontal: HorizontalAlignment
var vertical: VerticalAlignment
}
You'll lose the custom frame case, but I think that can be done with a size constraint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is worth it to lose the custom frame case – providing an "escape hatch" for people still seems valuable so we're not on the hook for implementing every needed case within this type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about replacing Corner with Alignment; seems about the same but added functionality for centering on a given side?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I think to do this "right" it's gonna take unifying some types between alignment guides, the aligned type, and this. Let's go ahead and merge this as is, and then I can follow up with that in another PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this is really the main sticking point for me in this PR. If we use Alignment it's almost exactly the same as SwiftUI's secondary views, but if we invent new types for this it moves us further away.
I guess I would be OK merging as-is as long as we follow up before we do another release? Alternatively, I could make a PR onto this branch with suggested changes, seeing as how I've worked with alignments already, I have a pretty good idea of the changes involved.
What about replacing Corner with Alignment; seems about the same but added functionality for centering on a given side?
It's not. The static cases have similar names, but alignments are opaque — resolving them gives you an x/y position in the coordinate space of the parent.
providing an "escape hatch" for people still seems valuable
I think the custom frame escape hatch is still achievable using Alignment if you wrap your content in .constrainedSize()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it – this makes sense. I'll do another PR into this one to separate the changes. I plan on keeping the outer positioning
though; and will replace corner with alignment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So something I don't quite understand looking at the existing alignments defined in Alignments.swift – is each of them provide a different edge – eg leading provides 0, center provides the center, and trailing provides the width – but in the case of eg the Aligned element, I need to ensure the contained wrapped element just buts up against the current edge – but this edge depends on the Alignment – but since the alignments are now just types, I can't switch over them easily... I think I'd run into the same issue here... Maybe this would be better to discuss tomorrow in 1:1? I feel like we're missing something to make this work everywhere, or I'm missing something obvious
/// | ||
/// See the `Decorate` element for more. | ||
/// | ||
public func decorate( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we expose this with background and overlay modifiers instead (maybe renaming the latter to disambiguate from the Overlay
element)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with this because of potential confusion – eg background sounds like I'm giving a background to my current element, and overlay is confused with Overlay
as you mention.
/// └───────────────────┘ | ||
/// ◀───────────────▶ ◀───────────────▶ | ||
/// ``` | ||
public struct Decorate : ProxyElement { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really close to an implementation of #49, with just a few API tweaks!
I'm going ahead and land this, we can update with the alignments later. |
* main: Bump versions to 0.21.0 Add additional ways to create and mutate an overlay, eg to ease conditional construction. Introduce conditionals on Element Prep 0.20.0 Add `Transformed` element
This PR introduces the
Decorate
element, which lets you put a decoration element above or below a content element, and position it depending on insets, corner, or custom. This is useful for drawing backgrounds behind components, drawing badges on elements, etc.