-
Notifications
You must be signed in to change notification settings - Fork 4
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
[Shapes] - Implement Ellipse (resolves #29) #30
[Shapes] - Implement Ellipse (resolves #29) #30
Conversation
width: 200, | ||
height: 100, | ||
child: Ellipse( | ||
fillGradient: LinearGradient( |
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 introduce a gradient API that matches the Swift UI API, too? I'm thinking that part of enabling Swift UI developers in Flutter is to make it as likely as possible that when developers start typing what they expect from Swift UI, they'll get the right thing in this package. It looks like Flutter already has LinearGradient
, which matches the name in Swift UI, but it looks like Swift UI has a different set of property names and enum values.
This might end up being a counterproductive idea, but I think the only way to know is to try it and then see whether its harder or easier to work with this package.
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.
Is your meaning to have the following properties on Ellipse (and every other shape) and define new classes to represent the gradient types?
fillLinearGradient: LinearGradient(...)
fillRadialGradient: RadialGradient(...)
fillEllipticalGradient: EllipticalGradient(...)
fillAngularGradient: AngularGradient(...)
fillConicGradient: ConicGradient(...)
strokeLinearGradient: LinearGradient(...)
strokeRadialGradient: RadialGradient(...)
strokeEllipticalGradient: EllipticalGradient(...)
strokeAngularGradient: AngularGradient(...)
strokeConicGradient: ConicGradient(...)
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.
No I meant that Swift UI appears to have properties inside of LinearGradient
such as startPoint
and endPoint
, with values such as .leading
and .trailing
. Those are different than the properties in Flutter's LinearGradient
. So I'm thinking we should create a swift_ui version of LinearGradient
that matches the properties and possible values of the Swift UI version.
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 we would keep the fillGradient
and strokeGradient
parameter names, but they would take a LinearGradient
matching SwiftUI that we would define ourselves, correct?
If so, I'm assuming LinearGradient
would implement an abstract Gradient
class so that we could add RadialGradient
, EllipticalGradient
, AngularGradient
and ConicGradient
as well. (Those are all SwiftUI gradient types).
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.
On second thought, let's go ahead and keep using Flutter's gradients. I'm a little worried that deep areas of Flutter might depends upon the Gradient
type and our class would be incompatible. We can re-evaluate later.
Should I approve this and merge it if I don't find anything else?
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 agree I think. Gradients aren't something people use often enough to require one specific API. I have to review how to do it every time I use them. The class names are similar enough in Flutter that a SwiftUI dev should be able to figure it out fairly easily.
You can merge if the rest looks ok.
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.
LGTM - Thanks.
This PR adds an
Ellipse
widget along with examples to the SwiftUI gallery and Flutter demo apps. Here are the comparisons side by side (SwiftUI is first):Issues of note
Unlike
Rectangle
, half the stroke line of an ellipse is painted outside of the ellipse area.SwiftUI clips the stroke of
Ellipse
when embedded directly in aVStack
. I had to manually ensure the SwiftUIVStack
was wide enough to contain the stroke width in order for this not to happen. This behavior is not matched in Flutter. The FlutterVStack
does not clip theEllipse
stroke by default:I'm not sure it's even worth trying to implement this behavior in Flutter since the Flutter default seems better. In any case, this is probably a
VStack
orFrame
issue rather than anEllipse
issue, so I'm not attempting to solve it in this PR.