-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Added the superellipse (a.k.a. squircle) shape to flutter. #26295
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
Conversation
…eeded to recreate some cupertino components, e.g. buttons in pixel-perfect detail (issue #13914).
…sed an error that doesn't seem to appear anymore.
|
Thanks for this, I think it's good overall. Do you think you could add some golden tests for this? https://github.com/flutter/flutter/wiki/Writing-a-golden-file-test-for-package%3Aflutter I know we don't have other goldens for the other shapes, but my concern is that this one is a bit more sensitive and we'll want to make sure we don't end up messing up the math in it. It will also make it easier for future reviewers if anything has to be tweaked here. |
|
You're probably right that this shape is a bit sensitive, are there any other tests that use goldens or some guide I can take a look at? I'm a bit new to writing tests. |
|
Create a pr on flutter/goldens with your changes (from your own fork of the repo). Please link it to this one for reference! |
xster
left a comment
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.
Very nice. Thanks for the PR!
|
Just created the PR in flutter/goldens (flutter/goldens#23). |
|
This is blocked by #24728 because of the way our goldens work right now |
|
Can you try with 034b2a540bc46375cf0c175a0fd512dcd46971e0 for the golden hash? That should work. I've reverted the not-ready goldens. |
|
I changed the hash and the test works just fine for me. |
|
LGTM! |
|
Please add yourself to AUTHORS |
|
Done 👍 |
|
eeeeeeeeeeeee! |
|
@Salby Not to diminish your work, but after playing around with this shape for a few minutes, I'm not sure it's really ready for Flutter in it's current state. Right now, the shape you've added is neither a super ellipse (defined mathematically) nor a shape that approximates the an iOS icon for (or of the squircle-esque rounded rects they use in other parts of iOS) for all possible sizes of rects. It looks like you used the formula here? https://github.com/spoeken/io7icon/blob/master/index.html You were definitely close, but I don't think it's quite good enough yet. Going forward, I think it could make sense to define not a super ellipse (which would be quite difficult from a mathematical standpoint to even approximate correctly), but an iOS icon shape whose radius cannot be changed (but whose shape stays constant with an increase in edge length). I'm happy to take over finalizing this shape or if you want to, that is fine too, just let me know! |
|
Thank you for taking the time to look at my PR. I agree that it isn't really a superellipse. When I initially started experimenting with the shape I used the mathematical function to calculate it, but that didn't give a lot of control over the individual corners which is why I switched to calculating each corner by itself. I really like the idea with the iOS icon shape. Can you clarify what you mean when you say that the shape should stay constant with an increase in edge length? That would be appreciated. |
|
Sure, yah right now, from the way I used it, when I changed the value of the width and height from this example: https://flutterawesome.com/a-package-for-creating-superellipse-shapes-in-flutter/, the corners of the rendered shape stayed the same height in pixels, whereas with a super ellipse (or what we're trying to implement), you would expect the corners to grow in size proportionally to the change in width or height of the rendered box. (ie, increasing the height of the shape by 50% would increase the height of the curves by 50% as well). I'm also thinking as far as tests go, I think ideally, it could make sense to render this shape on top of a native iOS icon graphic so we can see exactly how close this new shape is to what would be rendered natively. Or at the very least, do a manual comparison in photoshop or something just so we know this is a closer approximation to the desired shape than the rounded rect (which is actually already pretty close!). |
…6295) * Added the superellipse (a.k.a. squircle) shape to flutter, which is needed to recreate some cupertino components, e.g. buttons in pixel-perfect detail (issue flutter#13914).
Added the superellipse (a.k.a. squircle) shape to flutter, which is needed to recreate some cupertino components (e.g. buttons) in pixel-perfect detail (issue #13914).