Skip to content
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 Rectangle widget (resolves #26) #28

Merged

Conversation

suragch
Copy link
Collaborator

@suragch suragch commented Jun 6, 2024

@matthew-carroll I'm going to create an initial PR now so that I can get feedback before continuing too far in a direction that we don't want to go.

This PR implements a minimal Rectangle widget in Flutter. Specifically you'll find the following additions to the repo:

  • Rectangle examples in the SwiftUI gallery app. I also added Shapes to the More bottom nav bar.
Screenshot 2024-06-06 at 12 04 12 Screenshot 2024-06-06 at 12 06 08 Screenshot 2024-06-06 at 12 06 26
  • Mirrored the navigation in the Flutter demo app, but only implemented two rectangle styles so far:
Screenshot 2024-06-06 at 12 07 50 Screenshot 2024-06-06 at 12 08 02 Screenshot 2024-06-06 at 12 08 08

Issues to consider

  • In SwiftUI, the fill is a ShapeStyle, which can be a color, gradient, or other pattern. I'm not really sure what the best way is to implement this in Flutter. I created a ShapeStyle class but maybe it would be better to just have color and gradient be properties on Rectangle. Also, ShapeStyle in SwiftUI doesn't have a color property. I think it might get color from the ForegroundStyle. It doesn't have a gradient property either. Instead it has a linearGradient, radialGradient, etc. That didn't seem very Flutter-like to add all of those properties when they can all be expressed with Gradient.
  • Should there be some more general Shape class that ties all of the shapes together, or should we implement each shape widget independently?
  • I haven't added tests yet. I'll do that after I'm a little more sure of the direction. I also have to learn how to make golden tests.

I don't think squashing is currently enabled for this repo. Could you enable it so I don't have to rebase the individual commits on my fork?

@matthew-carroll
Copy link
Contributor

I don't think squashing is currently enabled for this repo. Could you enable it so I don't have to rebase the individual commits on my fork?

That's a setting that doesn't apply until you merge this PR into the main repository. This repo shouldn't cause any restrictions with regard to what you're able to do with your own branch on your own machine. Let me know if there's a specific action you can't seem to execute.

@matthew-carroll
Copy link
Contributor

I created a ShapeStyle class but maybe it would be better to just have color and gradient be properties on Rectangle. Also, ShapeStyle in SwiftUI doesn't have a color property. I think it might get color from the ForegroundStyle. It doesn't have a gradient property either. Instead it has a linearGradient, radialGradient, etc. That didn't seem very Flutter-like to add all of those properties when they can all be expressed with Gradient.

Unless specific hurdles come up, we should reflect the same API that exists in Swift UI. Are there any details that you're blocked from replicating in this case?

Should there be some more general Shape class that ties all of the shapes together, or should we implement each shape widget independently?

Is there a corresponding Swift UI API that you're thinking of? If not, let's just stick to replicating the public Swift UI APIs. If at a later point we find an opportunity to de-duplicate a bunch of implementation details behind the scenes, then we can consider that.

@suragch
Copy link
Collaborator Author

suragch commented Jun 7, 2024

That's a setting that doesn't apply until you merge this PR into the main repository. This repo shouldn't cause any restrictions with regard to what you're able to do with your own branch on your own machine.

Ah, I see. That's good to know. I was thinking that having multiple commits here would make your review harder by having view the commits separately.

Unless specific hurdles come up, we should reflect the same API that exists in Swift UI. Are there any details that you're blocked from replicating in this case?

In SwiftUI, ShapeStyle is a protocol that both Color and Gradient conform to. That means that the fill modifier can take either a Color or Gradient.

Rectangle()
    .fill(Color.green)
Rectangle()
    .fill(LinearGradient(gradient: Gradient(colors: [.yellow, .orange]), startPoint: .leading, endPoint: .trailing))

However, in Dart, Color and Gradient don't implement any other interface, so this isn't possible:

Rectangle(
  fill: Colors.blue,
),
Rectangle(
  fill: LinearGradient(colors: [Colors.yellow, Colors.orange]),
),

In my current PR I created a ShapeStyle class that could wrap a Color or Gradient:

Rectangle(
  fill: ShapeStyle(
    color: Colors.blue,
  ),
),
Rectangle(
  fill: ShapeStyle(
    gradient: LinearGradient(
      colors: [Colors.yellow, Colors.orange],
    ),
  ),
),

Is that the best way to do it given the constraints?

After thinking more about it, making Color an unnamed parameter would be better:

Rectangle(
  fill: ShapeStyle(Colors.blue),
),

Is there a corresponding Swift UI API that you're thinking of? If not, let's just stick to replicating the public Swift UI APIs. If at a later point we find an opportunity to de-duplicate a bunch of implementation details behind the scenes, then we can consider that.

There is a Shape protocol with the following types that conform to it:

But I agree that we should just implement the concrete shapes themselves for now.

Conclusion

Unless you have a new idea, I'm going to make Color an unnamed optional parameter of ShapeStyle and then continue in this general direction to finish the basic Rectangle implementation (i.e., stroke will also take a ShapeStyle).

@suragch
Copy link
Collaborator Author

suragch commented Jun 8, 2024

@matthew-carroll Ready for a final review.

The SwiftUI Rectangle examples are on the left and the Flutter ones are one the right:

Screenshot 2024-06-08 at 10 59 08 Screenshot 2024-06-08 at 10 59 24

I dropped the cornerRadius and shadow examples because these are view-level modifications in SwiftUI, not shape-level ones. In the future we'll need to create composable CornerRadius and Shadow widgets to implement those.

I mentioned my intention previously to have color be an optional unnamed parameter of ShapeStyle:

Rectangle(
  fill: ShapeStyle(Colors.blue),
),

However, it isn't possible in Dart to mix optional positional and named parameters. Upvote this issue if you think it should be.

So I'm reverting to the previous version:

Rectangle(
  fill: ShapeStyle(
    color: Colors.blue,
  ),
),

Originally I implemented Rectangle with a Container but the BoxDecoration didn't make gradient strokes very convenient so I replaced it with a CustomPainter.

@matthew-carroll
Copy link
Contributor

@suragch is there anything that the ShapeStyle is doing beyond just holding the color/gradient?

If not, is there any reason not to take a color or gradient directly within the Rectangle widget?

One option there would be to use two properties: fillColor, fillGradient.

I'm open to multiple approaches, but I wanted to check the rationale for ShapeStyle.

@suragch
Copy link
Collaborator Author

suragch commented Jun 11, 2024

@matthew-carroll The sole purpose for ShapeStyle is to attempt to match the type system in SwiftUI. It currently does nothiing more than wrap color and gradient. As a Flutter dev, I would much more prefer to use fillColor and fillGradient directly under Rectangle and just drop ShapeStyle. I think this would also be intuitive enough for SwiftUI devs.

@matthew-carroll
Copy link
Contributor

Sounds good. Let's do that.

@suragch
Copy link
Collaborator Author

suragch commented Jun 11, 2024

@matthew-carroll I removed ShapeStyle in favor of fillColor, fillGradient, strokeColor, and strokeGradient parameters on Rectangle.

There is no visual difference in the painted rectangles.

.gitignore Outdated Show resolved Hide resolved
lib/src/layout_adjustments/frame.dart Show resolved Hide resolved
lib/src/shapes/rectangle.dart Outdated Show resolved Hide resolved
test/shapes/rectangle_test.dart Outdated Show resolved Hide resolved
@suragch
Copy link
Collaborator Author

suragch commented Jun 12, 2024

@matthew-carroll You're suggestions are implemented now. Let me know if there are any further adjustments to make.

@@ -0,0 +1,117 @@
enum HorizontalAlignment {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that you moved HorizontalAlignment and VerticalAlignment into this file. Any reason why? Is that because you're preparing for the alignments to be used by widgets other than HStack and Vstack?

As for the directory name - does the term layout_adjustments exist in Swift UI? If not, I'd say this file can go in the layout directory.

Copy link
Collaborator Author

@suragch suragch Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alignment in SwiftUI can be defined in terms of HorizontalAlignment and VerticalAlignment so that's why I moved them to a common file rather than importing HStack and VStack into the Frame file.

The layout_adjustments already existed for the Frame widget, so that's why I put alignment.dart there. That sounds good to move alignment.dart to the layout directory. I assume I should move frame.dart there too?

lib/src/layout_adjustments/alignment.dart Outdated Show resolved Hide resolved
lib/src/layout_adjustments/frame.dart Outdated Show resolved Hide resolved
lib/src/layout_adjustments/frame.dart Outdated Show resolved Hide resolved
lib/src/shapes/rectangle.dart Outdated Show resolved Hide resolved
@suragch
Copy link
Collaborator Author

suragch commented Jun 12, 2024

@matthew-carroll Assuming the frame.dart move is correct, I've applied the new set of suggestions.

Copy link
Contributor

@matthew-carroll matthew-carroll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - Thanks.

@matthew-carroll matthew-carroll merged commit 8ee6660 into Flutter-Bounty-Hunters:main Jun 12, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants