Skip to content

Conversation

@Salby
Copy link
Contributor

@Salby Salby commented Jan 9, 2019

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).

…eeded to recreate some cupertino components, e.g. buttons in pixel-perfect detail (issue #13914).
@zoechi zoechi added the framework flutter/packages/flutter repository. See also f: labels. label Jan 9, 2019
@dnfield dnfield added a: fidelity Matching the OEM platforms better f: cupertino flutter/packages/flutter/cupertino repository labels Jan 9, 2019
…sed an error that doesn't seem to appear anymore.
@dnfield
Copy link
Contributor

dnfield commented Jan 9, 2019

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.

@Salby
Copy link
Contributor Author

Salby commented Jan 9, 2019

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.

@Salby
Copy link
Contributor Author

Salby commented Jan 9, 2019

I've written a golden test that works now.

According to the link @dnfield posted I have to push the golden image to the flutter/goldens repo, but I don't have permission to do that. Will someone else do it or will I be granted permission to do so?

@dnfield
Copy link
Contributor

dnfield commented Jan 9, 2019

Create a pr on flutter/goldens with your changes (from your own fork of the repo). Please link it to this one for reference!

Copy link
Member

@xster xster left a 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!

@Salby
Copy link
Contributor Author

Salby commented Jan 9, 2019

Just created the PR in flutter/goldens (flutter/goldens#23).

@dnfield
Copy link
Contributor

dnfield commented Jan 9, 2019

This is blocked by #24728 because of the way our goldens work right now

@dnfield
Copy link
Contributor

dnfield commented Jan 10, 2019

Can you try with 034b2a540bc46375cf0c175a0fd512dcd46971e0 for the golden hash? That should work. I've reverted the not-ready goldens.

@Salby
Copy link
Contributor Author

Salby commented Jan 10, 2019

I changed the hash and the test works just fine for me.

@dnfield
Copy link
Contributor

dnfield commented Jan 10, 2019

LGTM!

@dnfield
Copy link
Contributor

dnfield commented Jan 10, 2019

Please add yourself to AUTHORS

@Salby
Copy link
Contributor Author

Salby commented Jan 10, 2019

Done 👍

@dnfield dnfield merged commit 6c6fdaf into flutter:master Jan 10, 2019
@lukepighetti
Copy link
Contributor

eeeeeeeeeeeee!

@jslavitz
Copy link
Contributor

@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!

@Salby
Copy link
Contributor Author

Salby commented Jan 24, 2019

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.

@jslavitz
Copy link
Contributor

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!).

@Salby
Copy link
Contributor Author

Salby commented Jan 24, 2019

I think that was the original behaviour back when I was using the mathematical way of calculating the shape, but in my opinion it just looks stretched? Maybe it's just me.

Square vs Rectangle.

kangwang1988 pushed a commit to XianyuTech/flutter that referenced this pull request Feb 12, 2019
…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).
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

a: fidelity Matching the OEM platforms better f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants