Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Introduce Decorate element #178
Changes from 5 commits
255fce9
8c5bb7d
1551bc6
c87368b
c53469b
66da941
3d002f3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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!
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: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 anAlignment
type? I culled it from the alignment guides stuff because it was unused, but it looks like this: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.
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.
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
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.