codegrue / flutter_material_pickers

A flutter package for displaying common picker dialogs.
https://pub.dev/packages/flutter_material_pickers
MIT License
98 stars 61 forks source link

Null Safety Support #25

Closed 6h4n3m closed 3 years ago

6h4n3m commented 3 years ago

Hey everyone,

So, with the stable release of Flutter 2.0, migrating packages to null safety is something we need to think about.

Currently, all packages that flutter_material_pickers depend on have stable/beta releases that do support null safety, so the only question is, is support for null satefy planned?

If not, I'm more than ready to fork and submit a PR, just do not want to duplicate work if someone is already on it.

codegrue commented 3 years ago

I would love for you to do a PR. I am tight on time currently.

6h4n3m commented 3 years ago

@codegrue I'll get right on it.. if any situations should arise that require me to choose between specific implementation scenarios I'll ask here.

codegrue commented 3 years ago

I'm not even sure what needs to change. Maybe just the pubspec yaml version number to support 1.17.0. It seemed to run fine with the beta of nullsafety.

6h4n3m commented 3 years ago

Null safety migrated packages are usable by non-null safe apps and packages... so that's why your upgrade to the intl package in the last commit for the example worked.

What needed to be done is the following: 1) Update all dependencies to their null-safety versions. 2) Convert all code to null safe (this is mainly related to function parameters/arguments). 3 Replace any old dependencies with other ones that support null safety.

I'll send you the PR shortly.

codegrue commented 3 years ago

Merged into 2.0, cleaned up a few items, but the styles of the time picker are incorrect. This is a core Flutter widget, so not sure why that would be messed up. Will try to dig in later but if you have any thoughts would love to know.

Edit: Date picker title color is wrong also... looks like they changed the theme dependencies for these two controls.

From DatePicker dialog:

// The header should use the primary color in light themes and surface color in dark
final bool isDark = colorScheme.brightness == Brightness.dark;
final Color primarySurfaceColor = isDark ? colorScheme.surface : colorScheme.primary;
final Color onPrimarySurfaceColor = isDark ? colorScheme.onSurface : colorScheme.onPrimary;

I really think the Flutter team screwed this one up because you can't independently change the dialog title color anymore... not sure where to go with this. Have to think it over before I do a release.

6h4n3m commented 3 years ago

I'll take a look at this and get back to you if I find a suitable solution.

Also, since it looks like this is a major release with breaking changes anyway, the ResponsiveDialog widget has two instances of FlatButton which has been deprecated. The reason I didn't replace them with TextButton is that there's a breaking change in the API surface.

TextButton doesn't accept a textColor parameter with a Color argument anymore but instead uses a style parameter with a ButtonStyle argument inside of which we have to use an implementation of MaterialStateProperty to change the text style.

It's up to you if we accept a Color like before (so package users do not need to change anything) and then we can do something like this:

TextButton(
  style: ButtonStyle(
    textStyle: MaterialStateProperty.resolveWith((states) => TextStyle(color: _buttonTextColor)),
    ),
    child: Text(widget.cancelText ?? localizations.cancelButtonLabel),
    onPressed: () => (widget.cancelPressed == null)
    ? Navigator.of(context).pop()
    : widget.cancelPressed!(),
    ),

But then again, we can accept a ButtonStyle and give users the ability to provide all the styles they want not just the textColor.

Let me know what you think about this and I'll get back to you about the Date/Time pickers.

codegrue commented 3 years ago

I noticed this and moved the color change to the actual text instead of the button. Please do a pull to get my changes.

6h4n3m commented 3 years ago

Yeah, just saw it... a more consice solution.

About the Date/Time picker, I think one option is:

Edit: forgot to mention, that optional parameter should have a default value of Theme.of(context)

End result would look like this:

showTimePicker(
  context: context,
  ...params,
  builder: (BuildContext context, Widget child) {
    return Theme(
      data: userThemeData,
       ),
      child: child,
    );
  },
);
codegrue commented 3 years ago

Okay i'm working on using the generic dialog wrapper to contain these. It seems for datepicker at least they exposed the widget inside the stock dialog so that can be reused. This will make this dialog consistent with the others.