Add support for inverting CircularNotchedRectangle to optionally be drawn on the bottom of a path #151386
Conversation
… bottom of a path
github-actions
bot
added
the
framework
arbaker2
changed the title
nate-thegrate
left a comment
nate-thegrate
left a comment
There was a problem hiding this comment.
There's a little bit that I don't like about this getOuterPath() method, but it's not related to the changes you've made here.
LGTM, thanks for the fantastic contribution!
Co-authored-by: Nate Wilson <[email protected]>
|
We'll be able to merge this pull request once it gets a second approval. No guarantees, but hopefully it'll be within a few days! |
Great! Thanks so much Nate! And I appreciate the update to the comments. Your update was much more clear and concise. |
|
Thanks for this good quality PR! I don't have access to the Google testing failures but It might be related to the added parameter because it will break The PR description points to #49973, can you explain how one can use this PR to get the shape proposed in #49973? |
It is strange to me that this test failed as the PR with 1 commit passed all tests. The only change in the second commit was a change in comments recommended by @nate-thegrate. There is not much detail on the test as to why it failed. Is there a way to re-run the checks to verify there is an issue? Also, this PR does not allow for the shape in #49973. I originally opened up an issue #151381 which was closed because it was too similar to #49973. So that is why this PR is linked to this issue. |
I agree this is confusing. FYI, this is not related to your second commit. To re-run the check I clicked on 'Details' and then 'Re-run' (I dont know if one have to be a team member to do this). Pushing an empty commit or using the Github 'Update with rebase' button are ways to trigger the whole checks. |
|
Great the check is green now. @goderbauer |
|
@bleroux thanks for bringing up the concern about breaking subclasses; it didn't even cross my mind till now. It looks like there isn't any benefit to subclassing That being said, there is a pretty straightforward way to mitigate subclass breakages: class CircularNotchedRectangle extends NotchedShape {
const CircularNotchedRectangle({this.inverted = false});
final bool inverted;
@override
Path getOuterPath(Rect host, Rect? guest) {
// ...
}
}Perhaps it'd be best to implement this way, since |
|
@nate-thegrate @bleroux I am aligned with this adjustment as well. Thanks for bringing up these helpful concerns and suggested improvements! I can work to add another commit with this implemented. |
… subclassing issues
…ctangleInverted merge upstream master based on ci.yaml validation failure
|
@nate-thegrate @bleroux let me know if these changes look good to you or if further updates are needed. Thanks so much! |
bleroux
left a comment
bleroux
left a comment
There was a problem hiding this comment.
LGTM!
Thanks for this quality PR and for your reactivity 🙏
|
@nate-thegrate @bleroux thank you both for your help reviewing this PR! |
nate-thegrate
added
the
autosubmit
…lly be drawn on the bottom of a path (flutter/flutter#151386)
…lly be drawn on the bottom of a path (flutter/flutter#151386)
…lly be drawn on the bottom of a path (flutter/flutter#151386)
…lly be drawn on the bottom of a path (flutter/flutter#151386)
…lly be drawn on the bottom of a path (flutter/flutter#151386)
…lly be drawn on the bottom of a path (flutter/flutter#151386)
…lly be drawn on the bottom of a path (flutter/flutter#151386)
…lly be drawn on the bottom of a path (flutter/flutter#151386)
…lly be drawn on the bottom of a path (flutter/flutter#151386)
…lly be drawn on the bottom of a path (flutter/flutter#151386)
…lly be drawn on the bottom of a path (flutter/flutter#151386)
…lly be drawn on the bottom of a path (flutter/flutter#151386)
…lly be drawn on the bottom of a path (flutter/flutter#151386)
…lly be drawn on the bottom of a path (flutter/flutter#151386)
…lly be drawn on the bottom of a path (flutter/flutter#151386)
…lly be drawn on the bottom of a path (flutter/flutter#151386)
…lly be drawn on the bottom of a path (flutter/flutter#151386)
…lly be drawn on the bottom of a path (flutter/flutter#151386)
…lly be drawn on the bottom of a path (flutter/flutter#151386)
…lly be drawn on the bottom of a path (flutter/flutter#151386)
…lly be drawn on the bottom of a path (flutter/flutter#151386)
…lly be drawn on the bottom of a path (flutter/flutter#151386)
…lly be drawn on the bottom of a path (flutter/flutter#151386)
…rawn on the bottom of a path (flutter#151386) This PR allows for an optional argument [inverted[ to be passed to the [getOuterPath] method of a CircularNotchedRectangle object in order to invert the notch for situations where it is desired to draw the notch on the bottom of the path. This allows both of the below paths in the below screenshot to be drawn and changes no default behavior.  *List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.* This add a feature similar to the one discussed in flutter#49973, original feature proposal flutter#151381 *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].*
3 participants
This PR allows for an optional argument [inverted[ to be passed to the [getOuterPath] method of a CircularNotchedRectangle object in order to invert the notch for situations where it is desired to draw the notch on the bottom of the path. This allows both of the below paths in the below screenshot to be drawn and changes no default behavior.
List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.
This add a feature similar to the one discussed in #49973, original feature proposal #151381
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.