Add aspectRatio modifier
#422
Conversation
carson-katri
added
SwiftUI compatibility
carson-katri
force-pushed
the
aspect-ratio
branch
2 times, most recently
from
d91dacf to
1f0c8f6
Compare
carson-katri
force-pushed
the
aspect-ratio
branch
from
1f0c8f6 to
3e5568e
Compare
|
|
||
| @inlinable | ||
| public init(aspectRatio: CGFloat?, contentMode: ContentMode) { | ||
| (self.aspectRatio, self.contentMode) = (aspectRatio, contentMode) |
There was a problem hiding this comment.
nit: from coding style perspective I'd prefer it to be on separate lines, just for consistency's sake with the rest of the codebase.
| } | ||
| } | ||
|
|
||
| private class ResizableProvider: AnyImageProviderBox { |
There was a problem hiding this comment.
A bit unsure about this class hierarchy, are we going to have more than one subclass of AnyImageProviderBox? Maybe we need to document this pattern in our docs? When exactly is the creation of separate box classes is warranted?
There was a problem hiding this comment.
We will likely have something like SystemImageProvider in the future.
There was a problem hiding this comment.
Ah, you mean for something like SF Symbols?
There was a problem hiding this comment.
Those will need to be resolved in the environment so they can access the font and accessibility settings, so I think it makes sense to have it be a token.
| @@ -17,27 +17,121 @@ | |||
|
|
|||
| import Foundation | |||
|
|
|||
| public struct Image: _PrimitiveView { | |||
| let label: Text? | |||
| public class AnyImageProviderBox: AnyTokenBox, Equatable { | |||
There was a problem hiding this comment.
Should this have an underscore prefix to indicate that this shouldn't be re-exported?
| public class AnyImageProviderBox: AnyTokenBox, Equatable { | |
| public class _AnyImageProviderBox: AnyTokenBox, Equatable { |
| assertSnapshot( | ||
| matching: Ellipse() | ||
| .fill(Color.purple) | ||
| .aspectRatio(0.75, contentMode: .fit) |
There was a problem hiding this comment.
The PR looks good to me otherwise, but I just noticed that you only test .fit here, can a case for .fill also be added?
This adds the
aspectRatio,scaledToFill, andscaledToFitmodifiers and theContentModeenum.I also added the
resizablemodifier toImage, and made it respect the specifiedaspectRatio.