-
Notifications
You must be signed in to change notification settings - Fork 27.2k
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
[Cupertino] Exclude border for last action item in CupertinoContextMenu
#92481
[Cupertino] Exclude border for last action item in CupertinoContextMenu
#92481
Conversation
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.
This looks like a good change just some feedback about small-stuff.
// The scale of the child at the time that the CupertinoContextMenu opens. | ||
// This value was eyeballed from a physical device running iOS 13.1.2. | ||
const double _kOpenScale = 1.1; | ||
|
||
// static const Color _kBackgroundColorPressed = Color(0xFFDDDDDD); | ||
const Color _kBackgroundColorPressed = CupertinoDynamicColor.withBrightness( |
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.
Assuming that the darkColor is correct - this looks like a good additional change. This PR's description and a regression test should cover this change.
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.
Did this leak from when you split it out from #92480?
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.
Yes, I wanted this to be merged after #92480 since border color also changes with the theme. Only reason I found out we've extra border because of the dark theme PR
83e9bfa
to
4bb664b
Compare
@HansMuller This needs to be merged after #92480 (it includes testing for dark mode) |
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
Unfortunately because of #92611 I can't retry the test that failed due to an infra issue.
|
@HansMuller |
Description
fixes #92182
Fixes last CupertinoContextMenu action item getting extra border
This problem surfaced when fixing #43211, the last divider more clearly visible in dark mode
Preview
Of course,
CupertinoContextMenu
appearance isn't exactly like native iOS. but that's unrelated to this PR. See #92260 for more details.Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.