Refine UIKit to SwiftUI Measurement Strategies #162
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
brynbodayle
force-pushed
the
bb-update-swiftui-measurement-strategies
branch
from
ae075d7 to
54c3a04
Compare
brynbodayle
force-pushed
the
bb-update-swiftui-measurement-strategies
branch
from
54c3a04 to
3226ed0
Compare
brynbodayle
force-pushed
the
bb-update-swiftui-measurement-strategies
branch
5 times, most recently
from
8519841 to
8a287ff
Compare
brynbodayle
force-pushed
the
bb-update-swiftui-measurement-strategies
branch
from
8a287ff to
2ca6dc4
Compare
brynbodayle
commented
Feb 21, 2024
| return nsView.measuredFittingSize | ||
| } | ||
| #endif | ||
| } | ||
| #endif | ||
|
|
||
| #if swift(>=5.7.1) // Proxy check for being built with the iOS 15 SDK |
There was a problem hiding this comment.
Was getting a build error until I bumped this swift version up
brynbodayle
commented
Feb 21, 2024
| // `UILabel.preferredMaxLayoutWidth`. | ||
| let intrinsicSize = content.systemLayoutFittingIntrinsicSize() | ||
|
|
||
| // If the view has a intrinsic width and contains a double layout pass subview, give it the |
There was a problem hiding this comment.
this extra measurement pass didn't seem necessary in theory or in practice
brynbodayle
commented
Feb 21, 2024
| @nonobjc | ||
| fileprivate func containsDoubleLayoutPassSubviews() -> Bool { | ||
| #if os(macOS) | ||
| return false | ||
| #else | ||
| var contains = false | ||
| if let label = self as? UILabel, label.preferredMaxLayoutWidth > 0 { | ||
| if let label = self as? UILabel, label.numberOfLines != 1 { |
There was a problem hiding this comment.
We can simply check if the label is configured to show multiple lines. No need depending on an extra layout pass to determine if the label can wrap.
brynbodayle
force-pushed
the
bb-update-swiftui-measurement-strategies
branch
3 times, most recently
from
7c76ca3 to
b331cfe
Compare
brynbodayle
force-pushed
the
bb-update-swiftui-measurement-strategies
branch
from
b331cfe to
2a355a1
Compare
brynbodayle
force-pushed
the
bb-update-swiftui-measurement-strategies
branch
from
2a355a1 to
35215ce
Compare
|
This is ready for review now. Happo diffs look more correct when testing in the Airbnb app. |
bryankeller
reviewed
Mar 1, 2024
| /// - If the view contains `UILabel` subviews that require a double layout pass as determined by | ||
| /// a `preferredMaxLayoutWidth` that's greater than zero after a layout, then the view will | ||
| /// default to `intrinsicHeightProposedWidth` to allow the labels to wrap. | ||
| /// - If the view contains `UILabel` subviews that require a double layout pass as determined by support multiple lines of text |
There was a problem hiding this comment.
wording is a little messed up
bryankeller
approved these changes
Mar 1, 2024
bryankeller
reviewed
Mar 1, 2024
| @@ -22,6 +22,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
| approach to resolve an issue that could cause collection view cells to layout with | |||
| unexpected dimensions | |||
| - Made new layout-based SwiftUI cell rendering option the default. | |||
| - Fixed an issue where a UIKit view bridged to SwiftUI that wraps would always take up the proposed | |||
There was a problem hiding this comment.
I guess we fixed it for AppKit too?
There was a problem hiding this comment.
TBD, waiting for some updates from @calda since Epoxy iOS doesn't officially support native macOS, but Lottie iOS pulls in the code and uses it directly for macOS.
bryankeller
reviewed
Mar 1, 2024
| extension ProposedViewSize { | ||
| /// Creates a size by capping infinite values to a significantly large value and replacing `nil`s | ||
| /// with `UIView.noIntrinsicMetric` | ||
| var viewTypeValue: CGSize { |
There was a problem hiding this comment.
I don't really understand the "viewType" part of this name
There was a problem hiding this comment.
Added a comment to clarify
thedrick
approved these changes
Mar 1, 2024
brynbodayle
added a commit
that referenced
this pull request
Mar 14, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Change summary
How was it tested?
How did you verify that this change accomplished what you expected? Add more detail as needed.
Pull request checklist
All items in this checklist must be completed before a pull request will be reviewed.
CollectionViewConfigurationCHANGELOG.mdentry in the "Unreleased" section for any library changes