-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Manually select desired controls #794
base: master
Are you sure you want to change the base?
Manually select desired controls #794
Conversation
class AdaptiveControls extends StatelessWidget { | ||
const AdaptiveControls({ | ||
Key? key, | ||
required this.controlsType, |
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.
Please consider making this nullable and optional, so that it reverts to this class to determine the type of controls to render.
switch (controlsType) { | ||
case ControlsType.cupertino: | ||
return const CupertinoControls( | ||
backgroundColor: Color.fromRGBO(41, 41, 41, 0.7), | ||
iconColor: Color.fromARGB(255, 200, 200, 200), | ||
); | ||
case ControlsType.material: | ||
return const MaterialControls(); | ||
case ControlsType.materialDesktop: | ||
return const MaterialDesktopControls(); | ||
case ControlsType.adaptive: | ||
break; | ||
} | ||
|
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.
Instead of having to duplicate code in this method, consider doing the following:
- Instead of using a separate enum for this, you may want to use an optional and nullable
TargetPlatform
when creating this widget. That way if it exists, use that value, otherwise, fallback to the one from the theme. - Then you can determine the effective
TargetPlatform
to use based on the above and pass that into the switch statement:
final effectiveTargetPlatform = customPlatform ?? Theme.of(context).platform;
switch (effectiveTargetPlatform) {
//...
}
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.
@vinaykumarrock986612 any updates?
Currently, the controls are displayed based on the platform, and there's an option to choose build custom controls. But there's no option to use Cupertino controls as default if I want.
Added a high-level attribute to select desired controls type