-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Add StrokeAlign to Border #102112
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
Add StrokeAlign to Border #102112
Conversation
cdb7295 to
75b6580
Compare
|
I just committed big changes refactoring my PR (it was already ready for review, but while making another PR I learned a few tricks and came back here):
|
gspencergoog
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.
Thanks for the PR, this is really a cool addition!
Just some minor things, I think it looks pretty good overall.
gspencergoog
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.
|
Hmm. Looks like you got hit by a random CI issue where it didn't launch all the checks. To fix it, push an empty commit (i.e. use |
|
It seems tree is broken by something else (all PR are broken). Maybe you should mark as "wait tree to go green". |
|
Yes, the tree is also broken at the moment, but because the "ci.yaml" step failed, you'll need to push an empty commit. That's the step that starts most of the CI checks. |
|
I just pushed, but I think someone still didn't fix the ci.yaml issue. I can push again when they do. |
|
rebasing to the top |
|
Okay, looks like the checks fired off this time. And the tree is green, so once the tests pass, this will go in! |
|
This pull request is not suitable for automatic merging in its current state.
|
|
Ahh, sorry, I forgot we'll need a second review. Let me see who can take a look. |
darrenaustin
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.
LGTM as well. Just a couple of readability comments below. Nice work. Some tricky cases here.
|
|
||
| bool get _strokeAlignIsUniform { | ||
| final StrokeAlign topStrokeAlign = top.strokeAlign; | ||
| return start.strokeAlign == topStrokeAlign && bottom.strokeAlign == topStrokeAlign && end.strokeAlign == topStrokeAlign; |
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.
| return start.strokeAlign == topStrokeAlign && bottom.strokeAlign == topStrokeAlign && end.strokeAlign == topStrokeAlign; | |
| return start.strokeAlign == topStrokeAlign | |
| && bottom.strokeAlign == topStrokeAlign | |
| && end.strokeAlign == topStrokeAlign; |
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.
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.
It is funny you prefer && at the start than end. The file has both patterns, so it doesn't matter, I'll use exactly the way you wrote. But first time I see someone doing that.
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.
We do it because with four spaces before the &&, and one after, it makes all of the terms line up under the return. It's not something we apply religiously, only in cases where there are a lot of conditions stacked up.
|
I fixed it, thanks Darren! |
- Fix BoxDecoration + BorderRadius(0) (which is different than BoxDecoration without a BorderRadius). - Fix getInnerPath for CircleBorder.
Remove variable that may not always be needed.
Style fix.
Add indentation.




Fix #88621.
This is not a breaking change, however, we probably want BorderSide.toString() to include the new StrokeAlign property, which kind of makes it a breaking change. I had to update 20 tests. We could live without this, it works the same without toString(), but I think it was the correct thing to do.
Left, Figma. Right, Flutter:

The change is relatively simple and allows nice things in the future, such as rings which are popular in Tailwind.
Important to know:
Screen.Recording.2022-04-18.at.02.31.43.mov
Border(left: BorderSide(strokeAlign: StrokeAlign.outside)) is not supported because its implementation is different (and we probably don't want this to happen, like BorderRadius is also forbidden).
Border and all OutlinedBorders were updated so it gets half the padding on center and no padding at all at outside.
Code sample to play with my changes: