Create cupertino checkbox by MitchellGoodwin · Pull Request #122244 · flutter/flutter

MitchellGoodwin · 2023-03-08T23:08:17Z

Addresses: #61789.

Adds a macOS/iOS styled checkbox widget to Cupertino. Works with keyboard focus, and keyboard interactions. The goal is to make this widget adaptable with the Material Checkbox.

Notably SwiftUI does not provide checkboxes for iOS development, only macOS, so our comparison tool could not be used. So we have to compare with desktop checkboxes and the checkboxes in the Human Interface Guidelines.

Some screenshots of the checkbox in the macOS settings, and from the Human Interface Guidelines:

Here is the CupertinoCheckbox in action, both with mouse and keyboard interactions:

Screen.Recording.2023-03-08.at.3.06.53.PM.mov

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

MitchellGoodwin · 2023-03-08T23:10:02Z

packages/flutter/lib/src/cupertino/checkbox.dart

I'm not confident on the wording here. I would like to warn developers of Apple's guidance, without discouraging them from doing what they would like to do.

guidezpl · 2023-03-09T08:28:22Z

FYI for iOS it's in the HIG but only implicitly (e.g. in tables https://developer.apple.com/design/human-interface-guidelines/components/layout-and-organization/lists-and-tables/#platform-considerations)

bernaferrari · 2023-03-09T17:48:34Z

@GroovinChip might be the best person to give feedback on this, he made the macOS checkbox on Flutter.

gspencergoog

Just made an initial pass. Mostly it looks pretty good.

gspencergoog · 2023-03-10T00:25:10Z

packages/flutter/lib/src/cupertino/checkbox.dart

How about this?

Suggested change

/// Note: in the Apple ecosystem, checkboxes are encouraged to only be used in

/// macOS, not iOS. If a multi-selection component is needed, iOS encourages the

/// developer to use Switches, or [CupertinoSwitch] instead, or find a creative

/// custom solution.

/// In the Apple Human Interface Guidelines (HIG), checkboxes are encouraged for use on macOS, but not on iOS.

/// If a multi-selection component is needed on iOS, the HIG encourages the developer to use switches ([CupertinoSwitch] in Flutter)

/// instead, or to find a creative custom solution.

Does it actively discourage checkboxes on iOS? If not, then I'd just say that "but is silent about their use on iOS".

And probably link the appropriate HIG guidelines. Also note that (lol!) our style guide discourages empty prose like "Note" "Note that", etc.

gspencergoog · 2023-03-10T00:27:41Z

packages/flutter/lib/src/cupertino/checkbox.dart

Does it assert? If so, you should mention that it asserts in debug mode.

gspencergoog · 2023-03-10T00:28:43Z

packages/flutter/lib/src/cupertino/checkbox.dart

Suggested change

/// true this callback cycle from false to true to null.

/// true this callback cycle from false to true to null and back to false again.

gspencergoog · 2023-03-10T00:30:03Z

packages/flutter/lib/src/cupertino/checkbox.dart

Also mention if it asserts in debug mode.

gspencergoog · 2023-03-10T00:30:34Z

packages/flutter/lib/src/cupertino/checkbox.dart

Suggested change

/// If this property is null, then the side will be width 1.

/// If this property is null, then the side defaults to a width of 1.

gspencergoog · 2023-03-10T00:39:10Z

packages/flutter/lib/src/cupertino/checkbox.dart

Why can't these use locations for this use CupertinoCheckbox.width directly? It is also a const, and would show where it comes from.

gspencergoog · 2023-03-10T00:41:46Z

packages/flutter/lib/src/cupertino/checkbox.dart

These numbers in the offsets should be converted to constants to make it easier to read/understand them. Or you could comment on how the values are derived right here in a comment if this is the only instance of the number.

gspencergoog · 2023-03-10T00:43:36Z

packages/flutter/lib/src/cupertino/toggleable.dart

Suggested change

/// A mixin for [StatefulWidget]s that implement ios-themed toggleable

/// A mixin for [StatefulWidget]s that implement iOS-themed toggleable

gspencergoog · 2023-03-10T00:43:59Z

packages/flutter/lib/src/cupertino/toggleable.dart

Suggested change

/// The mixin implements the logic for toggling the control (e.g. when tapped).

/// The mixin implements the logic for toggling the control when tapped.

gspencergoog · 2023-03-10T00:44:25Z

packages/flutter/lib/src/cupertino/toggleable.dart

Suggested change

/// This mixin is used to implement the cupertino components for [Checkbox]

/// This mixin is used to implement the Cupertino components for [Checkbox]

gspencergoog · 2023-03-10T01:41:18Z

packages/flutter/lib/src/cupertino/checkbox.dart

Remove extra blank line

gspencergoog · 2023-03-10T01:41:54Z

packages/flutter/lib/src/cupertino/checkbox.dart

Adaptive widgets don't need to be the same size, and I think it's probably better to have it conform to the standard interactive size for Cupertino, since that should fit better with other Cupertino widgets. It's not much different anyhow.

gspencergoog · 2023-03-16T20:47:43Z

packages/flutter/lib/src/cupertino/checkbox.dart

Suggested change

/// The color used if the checkbox is inactive. By default,

/// [CupertinoColors.inactiveGray] is used.

/// The color used if the checkbox is inactive.

///

/// By default, [CupertinoColors.inactiveGray] is used.

The first paragraph is used as a short description in the docs, and should only be one sentence long.

https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#dartdoc-specific-requirements

gspencergoog · 2023-03-16T20:49:50Z

packages/flutter/lib/src/cupertino/checkbox.dart

Suggested change

// The ratios for the offsets below where found from looking at the checkbox

// The ratios for the offsets below were found from looking at the checkbox

gspencergoog · 2023-03-16T23:15:20Z

packages/flutter/lib/src/cupertino/toggleable.dart

Suggested change

/// A mixin for [StatefulWidget]s that implement iOS-themed toggleable

/// A mixin for [StatefulWidget]s that implements iOS-themed toggleable

gspencergoog · 2023-03-16T23:16:18Z

packages/flutter/lib/src/cupertino/toggleable.dart

Suggested change

/// controls (e.g.[Checkbox]es).

/// controls (e.g. [CupertinoCheckbox]es).

Or describe how this class relates to the Material Checkbox.

gspencergoog · 2023-03-16T23:16:34Z

packages/flutter/lib/src/cupertino/toggleable.dart

Suggested change

/// The mixin implements the logic for toggling the control when tapped.

/// This mixin implements the logic for toggling the control when tapped.

gspencergoog · 2023-03-16T23:23:38Z

packages/flutter/lib/src/cupertino/toggleable.dart

Suggested change

/// If value is true then the control "active" (checked, on, or selected). If

/// If [value] is true then the control "active" (checked, on, or selected). If

gspencergoog · 2023-03-16T23:26:01Z

packages/flutter/lib/src/cupertino/toggleable.dart

You can remove this entire function, since it's only calling super.

gspencergoog · 2023-03-16T23:27:07Z

packages/flutter/lib/src/cupertino/toggleable.dart

Suggested change

/// the Toggleable. In their [paint] method subclasses may call

/// the Toggleable. In their [paint] method, subclasses may call

I'm going to remove this sentence entirely, because I missed that I had deleted the method this is referring too.

gspencergoog · 2023-03-16T23:27:58Z

packages/flutter/lib/src/cupertino/checkbox.dart

Suggested change

/// If true the checkbox's [value] can be true, false, or null.

/// If true, the checkbox's [value] can be true, false, or null.

gspencergoog · 2023-03-16T23:31:27Z

packages/flutter/lib/src/cupertino/checkbox.dart

Suggested change

/// If tristate is false (the default), [value] is asseted to not be null.

/// If tristate is false (the default), [value] must not be null, and [onChanged] will only toggle between true and false.

MitchellGoodwin · 2023-03-17T17:46:54Z

@gspencergoog this is ready for review again 👍

gspencergoog

gspencergoog · 2023-03-20T20:40:23Z

packages/flutter/lib/src/cupertino/checkbox.dart

Maybe describe what this is a link to? (or make it into an actual Markdown link to the descriptive text?)

gspencergoog · 2023-03-20T20:41:44Z

packages/flutter/lib/src/cupertino/checkbox.dart

Suggested change

/// Creates a macOS styled checkbox.

/// Creates a macOS-style checkbox.

Just a tiny nit: when I read this the first time, I read it as a "styled checkbox" for macOS.

gspencergoog · 2023-03-20T20:44:53Z

packages/flutter/lib/src/cupertino/checkbox.dart

Suggested change

/// If this property is null, then the side defaults to a width of 1.

/// If this property is null, then the side defaults to a one pixel wide

/// black, solid border.

gspencergoog · 2023-03-20T20:50:26Z

packages/flutter/lib/src/cupertino/checkbox.dart

Too bad we have to cast here (but we do). Someday we should add a asOffset to Size and a asSize to Offset.

flutter-dashboard bot added f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels. labels Mar 8, 2023

MitchellGoodwin commented Mar 8, 2023

View reviewed changes

MitchellGoodwin marked this pull request as ready for review March 8, 2023 23:52

MitchellGoodwin requested a review from gspencergoog March 9, 2023 23:54

gspencergoog reviewed Mar 10, 2023

View reviewed changes

MitchellGoodwin force-pushed the create-cupertino-checkbox branch from b501bdf to 9beffb6 Compare March 10, 2023 19:35

gspencergoog reviewed Mar 16, 2023

View reviewed changes

MitchellGoodwin force-pushed the create-cupertino-checkbox branch from 9beffb6 to 0810582 Compare March 17, 2023 16:48

MitchellGoodwin force-pushed the create-cupertino-checkbox branch from 3916a7b to 9a5eec2 Compare March 20, 2023 16:18

gspencergoog approved these changes Mar 20, 2023

View reviewed changes

MitchellGoodwin force-pushed the create-cupertino-checkbox branch from 9a5eec2 to f084121 Compare March 20, 2023 21:16

MitchellGoodwin added 6 commits March 20, 2023 15:59

Start of setting up checkbox

c41acb9

Create CupertinoCheckbox

4c85463

Refactor documentation and constants

bbcb515

Update documentation

77bfec1

remove whitespace

5a2cf6f

Update docs

6ccacb8

MitchellGoodwin force-pushed the create-cupertino-checkbox branch from f084121 to 6ccacb8 Compare March 20, 2023 22:59

MitchellGoodwin added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 21, 2023

auto-submit bot merged commit 1ba3f99 into flutter:master Mar 21, 2023

engine-flutter-autoroll mentioned this pull request Mar 21, 2023

Roll Flutter from 23f5582ea654 to 1ba3f99bb0aa (21 revisions) flutter/packages#3505

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 21, 2023

1ba3f99 Create cupertino checkbox (flutter/flutter#122244)

5dcc501

HansMuller mentioned this pull request Mar 24, 2023

Flutter Framework Team: Notable Commits #121415

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023

1ba3f99 Create cupertino checkbox (flutter/flutter#122244)

da369e2

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023

1ba3f99 Create cupertino checkbox (flutter/flutter#122244)

2ffd06f

-/// Note: in the Apple ecosystem, checkboxes are encouraged to only be used in
-/// macOS, not iOS. If a multi-selection component is needed, iOS encourages the
-/// developer to use Switches, or [CupertinoSwitch] instead, or find a creative
-/// custom solution.
+/// In the Apple Human Interface Guidelines (HIG), checkboxes are encouraged for use on macOS, but not on iOS.
+/// If a multi-selection component is needed on iOS, the HIG encourages the developer to use switches ([CupertinoSwitch] in Flutter)
+/// instead, or to find a creative custom solution.

	/// true this callback cycle from false to true to null.
	/// true this callback cycle from false to true to null and back to false again.

	/// If this property is null, then the side will be width 1.
	/// If this property is null, then the side defaults to a width of 1.

	/// A mixin for [StatefulWidget]s that implement ios-themed toggleable
	/// A mixin for [StatefulWidget]s that implement iOS-themed toggleable

	/// The mixin implements the logic for toggling the control (e.g. when tapped).
	/// The mixin implements the logic for toggling the control when tapped.

	/// This mixin is used to implement the cupertino components for [Checkbox]
	/// This mixin is used to implement the Cupertino components for [Checkbox]

-  /// The color used if the checkbox is inactive. By default,
-  /// [CupertinoColors.inactiveGray] is used.
+  /// The color used if the checkbox is inactive.
+  ///
+  /// By default, [CupertinoColors.inactiveGray] is used.

	// The ratios for the offsets below where found from looking at the checkbox
	// The ratios for the offsets below were found from looking at the checkbox

	/// controls (e.g.[Checkbox]es).
	/// controls (e.g. [CupertinoCheckbox]es).

	/// The mixin implements the logic for toggling the control when tapped.
	/// This mixin implements the logic for toggling the control when tapped.

	/// If value is true then the control "active" (checked, on, or selected). If
	/// If [value] is true then the control "active" (checked, on, or selected). If

	/// the Toggleable. In their [paint] method subclasses may call
	/// the Toggleable. In their [paint] method, subclasses may call

	/// If true the checkbox's [value] can be true, false, or null.
	/// If true, the checkbox's [value] can be true, false, or null.

	/// If tristate is false (the default), [value] is asseted to not be null.
	/// If tristate is false (the default), [value] must not be null, and [onChanged] will only toggle between true and false.

	/// Creates a macOS styled checkbox.
	/// Creates a macOS-style checkbox.

	/// If this property is null, then the side defaults to a width of 1.
	/// If this property is null, then the side defaults to a one pixel wide
	/// black, solid border.

Conversation

MitchellGoodwin commented Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guidezpl commented Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bernaferrari commented Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gspencergoog left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gspencergoog Mar 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gspencergoog Mar 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MitchellGoodwin commented Mar 17, 2023

Uh oh!

gspencergoog left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

MitchellGoodwin commented Mar 8, 2023 •

edited

Loading

guidezpl commented Mar 9, 2023 •

edited

Loading

bernaferrari commented Mar 9, 2023 •

edited

Loading

gspencergoog Mar 10, 2023 •

edited

Loading

gspencergoog Mar 10, 2023 •

edited

Loading