Repository navigation
Roll Switch.adaptive changes into CupertinoSwitch - #149465
Conversation
github-actions
Bot
added
framework
victorsanni
changed the title
Switch.adaptive changes into CupertinoSwitch
If these properties did not exist before, is this a good opportunity to introduce them with WidgetStateProperties? The migration from MSP --> WSP was motivated by extending the concept to Cupertino. Should we be adopting that as we are revamping Cupertino widgets? Same for Separate question, some folks have asked - what do we do about the reimplemented iOS Switch in the material library (behind Switch.adaptive)? That is the only (I think?) existing implementation of an iOS widget in material, rather than .adaptive returning a Cupertino variant. We could decide to go back to Switch.adaptive and have it return CupertinoSwitch again while still using theme adaptations to help style, or maybe we will implement more material.adaptive widgets like Switch later. We're waiting on the results of a user study (just a couple of weeks away) to help make these decisions. I am curious though, @MitchellGoodwin @justinmc what are your thoughts here? |
|
On widget state properties in general, I think we should use them if there's multiple styling variations for a property. If the Cupertino version (according to native, not necessarily how we have it now) is always the same value, then in my opinion it's ok to not use widget state properties. On the adaptive Switch in the Material library, I think eventually we need to change it to align with everything else we are doing (or change everything else to match it). For now though I think it's a low priority until after the study, and we can keep it around. It's a good example case of that approach. |
@MitchellGoodwin I love this way of determining if we should use WidgetStateProperty or not. |
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
As others were discussing above, I agree with keeping the Material iOS implementation around until we've seen the user study and made a clear plan about how to handle this stuff.
Do you plan to make Switch.adaptive point to CupertinoSwitch in a subsequent PR?
Is it not practical to abstract any of this to a shared RawSwitch for sharing code? Just making sure we've thought about that. I'm guessing that a lot of the tests you added are mostly duplicated in Material, but they maybe could be deduplicated if there was a RawSwitch.
| /// Paints an iOS-style slider thumb. | ||
| /// | ||
| /// Used by [CupertinoSwitch] and [CupertinoSlider]. | ||
| /// Used by [CupertinoSlider]. |
There was a problem hiding this comment.
Why not use CupertinoThumbPainter? Is it out of date?
There was a problem hiding this comment.
CupertinoThumbPainter doesn't paint an image/icon on the thumb. I see some duplicated code, especially in the _paintCupertinoThumbShadowAndBorder method as compared to CupertinoThumbPainter's paint method. I thought it would be a challenge to refactor CupertinoThumbPainter to use this new implementation, so I just decided to not use it for now.
| ..rrect(color: const Color(0xffffffff)), | ||
| ); | ||
| }); | ||
| }, variant: TargetPlatformVariant.only(TargetPlatform.macOS)); |
There was a problem hiding this comment.
Why is this Mac only now?
There was a problem hiding this comment.
I had focus and focus tests only on macOS, I am not sure if that is good or bad (or what the consensus/best practice is).
There was a problem hiding this comment.
I have removed the macOS requirement for focus.
MitchellGoodwin
left a comment
MitchellGoodwin
left a comment
There was a problem hiding this comment.
A small nit and a question. Otherwise, this looks good to me.
| }); | ||
|
|
||
| /// Creates an object that paints an iOS-style switch thumb. | ||
| const CupertinoThumbPainter.switchThumb({ |
There was a problem hiding this comment.
Should we deprecate this? I wouldn't be surprised if nobody uses it, but it's technically a public API.
There was a problem hiding this comment.
That's true. It's not used anywhere else in the framework at least, but this would not be the way to deprecate it anyways.
QuncCccccc
left a comment
QuncCccccc
left a comment
There was a problem hiding this comment.
LGTM, assuming we only focus on having matched properties in material Switch and cupertino Switch in this PR:)!
victorsanni
added
the
autosubmit
5 participants
Brings over the changes from
Switch.adaptiveintoCupertinoSwitch.This change adds the following
Switchparameters toCupertinoSwitch:inactiveThumbColor,activeThumbImage,onActiveThumbImageError,inactiveThumbImage,onInactiveThumbImageError,trackOutlineColor,trackOutlineWidth,thumbIcon,mouseCursor.The following
Switchparameters are ignored:activeTrackColor: becauseactiveColorhas the same function.inactiveTrackColor: becausetrackColorhas the same function.materialTapTargetSize: because it is only applicable inMaterial.hoverColor,overlayColor,splashRadius: because these parameters configure the radial reaction overlay inMaterial, so are not applicable here.The following
CupertinoSwitchparameters which are absent fromSwitch.adaptiveare retained:onLabelColor,offLabelColor,applyThemetrackColorandthumbColorare of typeWidgetStatePropertyin MaterialSwitch, but are currently of typeColorinCupertinoSwitch. For backwards compatibility, both parameters are kept asColors, but can be resolved in differentWidgetStates usingWidgetStateColor.resolve().This PR does not apply any fidelity updates to
CupertinoSwitch.Fixes #149275
Related PRs: #130425 #148804
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.