-
Notifications
You must be signed in to change notification settings - Fork 28.7k
Add backgroundColor
to Radio
#169415
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 backgroundColor
to Radio
#169415
Conversation
This PR is to kickstart the fixes for #168787 I would like to do it iteratively.
|
final Color activeBackgroundColor = | ||
widget.backgroundColor?.resolve(activeStates) ?? Colors.transparent; | ||
final Color inactiveBackgroundColor = | ||
widget.backgroundColor?.resolve(inactiveStates) ?? Colors.transparent; |
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.
I'm not sure how we want to handle those transparent colors? The other properties are using _RadioDefaultsM3
or _RadioDefaultsM2
as a fallbacks.
What is the best thing to do here?
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.
@QuncCccccc Is this something that could/should be in the Material token database? Or is this ok as-is?
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.
At some point the property will be in RadioTheme right? We should default to the property in RadioTheme instead
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.
I agree that it should ultimately be included in the theme, @chunhtai.
However, as I mentioned in this comment #169415 (comment), my intention was to approach this iteratively and address the theme in a future PR. This way, I can keep this PR (and future ones) small and focused.
Would that be acceptable to you? If not, could you please specify the minimal set of changes required for this PR?
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.
sgtm as iterative change. consider add a todo here with the link to the issue for adding themedata so we don't miss this
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.
Makes sense, I added a TODO in Add TODO
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.
final Color activeBackgroundColor = | ||
widget.backgroundColor?.resolve(activeStates) ?? Colors.transparent; | ||
final Color inactiveBackgroundColor = | ||
widget.backgroundColor?.resolve(inactiveStates) ?? Colors.transparent; |
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.
@QuncCccccc Is this something that could/should be in the Material token database? Or is this ok as-is?
d9e5ffd
to
ab03c42
Compare
@@ -106,6 +106,7 @@ class Radio<T> extends StatefulWidget { | |||
this.autofocus = false, | |||
this.enabled, | |||
this.groupRegistry, | |||
this.backgroundColor, |
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 should also add this to RadioListTile
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.
Similar to #169415 (comment), I was considering adding it with another PR. Would that be fine with you?
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
/// * [WidgetState.disabled]. | ||
/// | ||
/// If null, then it is transparent in all states. | ||
final MaterialStateProperty<Color?>? backgroundColor; |
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.
MaterialStateProperty is deprecated, use WidgetStateProperty
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.
I used WidgetStateProperty
in Use WidgetStateProperty and RadioGroup
final Color activeBackgroundColor = | ||
widget.backgroundColor?.resolve(activeStates) ?? Colors.transparent; | ||
final Color inactiveBackgroundColor = | ||
widget.backgroundColor?.resolve(inactiveStates) ?? Colors.transparent; |
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.
At some point the property will be in RadioTheme right? We should default to the property in RadioTheme instead
key: radioKey, | ||
value: 0, | ||
fillColor: backgroundColor, | ||
onChanged: |
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.
Use radioGroup instead
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.
I used RadioGroup
in Use WidgetStateProperty and RadioGroup
ab03c42
to
608a2e6
Compare
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
@@ -106,6 +106,7 @@ class Radio<T> extends StatefulWidget { | |||
this.autofocus = false, | |||
this.enabled, | |||
this.groupRegistry, | |||
this.backgroundColor, |
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
final Color activeBackgroundColor = | ||
widget.backgroundColor?.resolve(activeStates) ?? Colors.transparent; | ||
final Color inactiveBackgroundColor = | ||
widget.backgroundColor?.resolve(inactiveStates) ?? Colors.transparent; |
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.
sgtm as iterative change. consider add a todo here with the link to the issue for adding themedata so we don't miss this
608a2e6
to
343aca3
Compare
Part of #168787
Adds
backgroundColor
property toRadio
This code sample is now possible:
Example
Screen.Recording.2025-05-24.at.10.18.04.PM.mov
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.