rydmike
commented
3 years ago Hi @RymBenJmaa, thanks for your contribution to FlexColorPicker.
It was a bit tricky too see exactly what you wanted to merge when so many lines that are not actually changed as a part of it, are also changed in the PR. It seemed to be just many lines being on one line, instead of formatted with dartfmt rules that had them on two lines before.
Using dartfmt means no line can be over 80 chars. If a package is not formatted according to dartfmt rules its "Pub Points" score on pub.dev get penalized, which is why it "kind of" have to be used for packages, at least if you care about the score. I know it is not always so pretty with lines that cannot be over 80chars, but it is what it, for package's at least.
I know... I should add some guides for pull requests.
I was going to ask if you could possibly clean it up with dartfmt and update the pull request. But since I think I got the gist, I think I can probably merge it and autoformat it back, no worries about that.
Still a few questions though, don't get me wrong as such the additions looked nice and welcome. I also at the same time want to make sure I caught all the improvements you are proposing correctly due to all the lines that were changed due to formatting, making it more difficult to read.
1. Style the color code prefix separately from the value.
You propose to be able to style the prefix of the color code, so that it can be made in another style than the color code entry. This sounds good to me.
Two small review topics with this line:
prefixStyle: widget.colorCodePrefixStyle ?? Theme.of(context).textTheme.body2,
First, body2 is a deprecated property, it should use bodyText2 instead. I know some of my comments mentioned body2 as well, I fixed that, actual code was already with bodyText2. However, it is also not relevant, since I don't think we need a fallback style at all in this case. Why not?
Well, if it is null, then it will via the SDK itself default to the style of the color code TextField, which is what we want if it is null.
This is what it does now, having it default to Theme.of(context).textTheme.bodyText2 would actually be a breaking change, since it would then not default to the style of color code TextField, if it happens to be defined to something else than bodyText2, and colorCodePrefixStyle is not defined. So I think we just remove the null fall back for this case.
With that we can also this in the property comment say:
/// The TextStyle of the prefix of the color code.
///
/// Defaults to [colorCodeTextStyle], if not defined.
final TextStyle colorCodePrefixStyle;2. Expose the thumb color of the CupertinoSlidingSegmentedControl that selects active picker.
Sure this is doable too.
One reason why I did not do it before, is that there is no direct way to set the text color of the selected picker in the CupertinoSlidingSegmentedControl to always use a correct onColor for the thumb color. So the slider will only look good by default if the thumbColor is of Brightness light in light mode and of dark, in dark mode. It is possible to do the needed correction with logic in the segmented controls content, more about that later. The reason why it does not exists is I just never got around to adding this needed extra tweak.
One thing though with the thumbColor, I think in this case we do need a fallback. Well at least in the null safe version, and it needs to be to the past SDK defaults to not break past colors. If it is left as null, we would be passing in null here:
thumbColor: widget.selectedPickerTypeColor
The CupertinoSlidingSegmentedControl thumbColor in SDK has a named default value that it would override with null, which it cannot even do in the null safe version. It is not using an "if it was null" then use this value, type of check later in the code, that Material widgets typically do. And yeah the null safe SDK version does not accept nullable Color at all for thumbColor , so we need that fall back there. Well at least in null safe version of the SDK, I'm currently in it, but will swap to other version soon and check how it is there too.
In any case this fallback should be correct and OK for both current SDK and upcoming null safe version:
thumbColor: widget.selectedPickerTypeColor ??
const CupertinoDynamicColor.withBrightness(
color: Color(0xFFFFFFFF),
darkColor: Color(0xFF636366),
),So then back to the correct on color for the thumbColor, when a color is provided. Now if you make it a dark color in light theme its text style remain black, and needs to be so for the none selceted version, but selected needs white text:
The above does not satisfy text contrast requirements. So we need some logic to fix it, it needs something like this:
final Color _thumbColor = widget.selectedPickerTypeColor ??
const CupertinoDynamicColor.withBrightness(
color: Color(0xFFFFFFFF),
darkColor: Color(0xFF636366),
);
final Color _thumbOnColor =
ThemeData.estimateBrightnessForColor(_thumbColor) == Brightness.light
? Colors.black
: Colors.white;That we can use to modify the segmentTextStyle color with for the selected segment, like so:
style: activePicker == ColorPickerType.both
? segmentTextStyle.copyWith(color: _thumbOnColor)
: segmentTextStyle,For each picker type, thus ending up with this:
@override
Widget build(BuildContext context) {
// Set default text style for the segmented slider control.
final TextStyle segmentTextStyle =
(widget.pickerTypeTextStyle ?? Theme.of(context).textTheme.caption)!;
final Color _thumbColor = widget.selectedPickerTypeColor ??
const CupertinoDynamicColor.withBrightness(
color: Color(0xFFFFFFFF),
darkColor: Color(0xFF636366),
);
final Color _thumbOnColor =
ThemeData.estimateBrightnessForColor(_thumbColor) == Brightness.light
? Colors.black
: Colors.white;
final Map<ColorPickerType, Widget> pickerTypes = <ColorPickerType, Widget>{
if (pickersEnabled[ColorPickerType.both]!)
ColorPickerType.both: Padding(
padding: const EdgeInsets.all(5),
child: Text(
widget.pickerTypeLabels[ColorPickerType.both] ??
ColorPicker._selectBothLabel,
textAlign: TextAlign.center,
style: activePicker == ColorPickerType.both
? segmentTextStyle.copyWith(color: _thumbOnColor)
: segmentTextStyle,
),
),
if (pickersEnabled[ColorPickerType.primary]!)
ColorPickerType.primary: Padding(
padding: const EdgeInsets.all(5),
child: Text(
widget.pickerTypeLabels[ColorPickerType.primary] ??
ColorPicker._selectPrimaryLabel,
textAlign: TextAlign.center,
style: activePicker == ColorPickerType.primary
? segmentTextStyle.copyWith(color: _thumbOnColor)
: segmentTextStyle,
),
),
if (pickersEnabled[ColorPickerType.accent]!)
ColorPickerType.accent: Padding(
padding: const EdgeInsets.all(5),
child: Text(
widget.pickerTypeLabels[ColorPickerType.accent] ??
ColorPicker._selectAccentLabel,
textAlign: TextAlign.center,
style: activePicker == ColorPickerType.accent
? segmentTextStyle.copyWith(color: _thumbOnColor)
: segmentTextStyle,
),
),
if (pickersEnabled[ColorPickerType.bw]!)
ColorPickerType.bw: Padding(
padding: const EdgeInsets.all(5),
child: Text(
widget.pickerTypeLabels[ColorPickerType.bw] ??
ColorPicker._selectBlackWhiteLabel,
textAlign: TextAlign.center,
style: activePicker == ColorPickerType.bw
? segmentTextStyle.copyWith(color: _thumbOnColor)
: segmentTextStyle,
),
),
if (pickersEnabled[ColorPickerType.custom]!)
ColorPickerType.custom: Padding(
padding: const EdgeInsets.all(5),
child: Text(
widget.pickerTypeLabels[ColorPickerType.custom] ??
ColorPicker._selectCustomLabel,
textAlign: TextAlign.center,
style: activePicker == ColorPickerType.custom
? segmentTextStyle.copyWith(color: _thumbOnColor)
: segmentTextStyle,
),
),
if (pickersEnabled[ColorPickerType.wheel]!)
ColorPickerType.wheel: Padding(
padding: const EdgeInsets.all(5),
child: Text(
widget.pickerTypeLabels[ColorPickerType.wheel] ??
ColorPicker._selectWheelAnyLabel,
textAlign: TextAlign.center,
style: activePicker == ColorPickerType.wheel
? segmentTextStyle.copyWith(color: _thumbOnColor)
: segmentTextStyle,
),
),
};Which works with the text style regardless what thumb color is provided and in both light and dark theme mode.
Other info
Just for info, I'm also at the moment working on some updates for the FlexColorPicker.
The work is going on in the null-safe branch. I may port some of the new features back into the "legacy" before_null_safety branch. Needs to be done by hand, mostly.
When null safety goes stable in Dart and Flutter, the null-safe branch will become new master and before_null_safety will live on only as legacy version for potential bug fixes to 1.x.x version.
BTW, I already incorporated your proposed PR changes into the null-safe branch while testing them, together with my own above fixes for the null-safe variant.
I'm ready to merge your PR and into the none null-safe master and make the above discussed mods myself too after that. Or you can update your PR to included the fixes as well and preferably run dartfmt at the same time. In either case I want to merge your PR so you get mentioned as a contributor.
Other on-going changes:
WIP
- Use opacity/alpha and enable optional opacity slider/control.
- Paste enhancement. Can paste a copied color code to any picker page/view and it takes the pasted color string, moves to the picker where it can find it, last option is on the wheel if it is present it can always find it there. You can paste RRGGBB AARRGGBB or short web RGB hex value strings with, # or 0x or no prefix included, it parses the color value correctly regardless of format.
- Copy enhancement. There is a copy style parameter to control if #, 0x, with or without alpha, or no prefix at all in the copied color code string. I had intended to always use this setting as content for the prefix, but I think I will instead make it so that it is the default for the prefix, if not provided, but you can also give a custom one.
- Both of these impact the current prefix content, but they do no clash with styling it, so the style prop works here too.
- Optional title, like a a mini app bar with optional select, close(cancel) icon buttons when used in a dialog.
- Possible to not use the SELECT CANCEL buttons at all in the dialog, if the app bar version is preferred.
LATER
- Different picker selector as alternatives to the
CupertinoSlidingSegmentedControl. Why? It is a bit problematic as it offers no keyboard control on Web or Desktop, probably never will as it is not a part of its spec. Considering adding a cutom variant that looks a bit like it, but can be controlled with keyboard and also adding a Material ToggleButtons variant. - A new advanced picker type is also in the works.
- Custom overlay instead of a dialog, that open close to where you clicked on the color.
- Movable overlay.
- Maybe: Color pipette function to select a color shown on screen.
Releasing a new pub.dev package version with the new features?
I will release the new PR features in the pub.dev package when I add some of my own next step enhancements mentioned above as well. Will probably be a few days before that happens. So if you still want to adjust the PR a bit there is no hurry.
As said, I'm building and testing the new features in the null safe version and back porting some of them into "classic" current stable version when done, so a bit of extra steps. I won't be doing that for features going forward when null safety goes stable channel, but since some might like the new features also in none null safe projects for a while, I'm porting over at least the first batch of them.
RymBenJmaa
add custom style for code color prefix and custom color for the selected ColorPickerType