Add padding property to Checkbox#188258
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request adds a padding property to the Checkbox widget, which insets and proportionally scales the check mark and indeterminate dash. The review feedback suggests documenting that this padding is ignored on iOS and macOS when using Checkbox.adaptive, adding padding to CheckboxThemeData for global styling, correcting the documentation regarding its default value, and exposing the property in debugFillProperties for debugging.
| this.side, | ||
| this.isError = false, | ||
| this.semanticLabel, | ||
| this.padding, |
There was a problem hiding this comment.
Since Checkbox.adaptive delegates to CupertinoCheckbox on iOS and macOS, and CupertinoCheckbox does not currently support a padding property, this padding will be ignored on those platforms. Please document this platform-specific behavior in the padding property's API documentation or the Checkbox.adaptive constructor documentation so developers are aware of this limitation.
There was a problem hiding this comment.
The documentation update regarding the platform-specific behavior of Checkbox.adaptive is appreciated. I noticed a small issue in packages/flutter/lib/src/material/checkbox_theme.dart: there is a duplicate line in debugFillProperties where side is added twice. Please remove the redundant line.
| this.padding, | |
| properties.add(DiagnosticsProperty<BorderSide>('side', side, defaultValue: null)); | |
| properties.add(DiagnosticsProperty<EdgeInsetsGeometry>('padding', padding, defaultValue: null)); |
There was a problem hiding this comment.
The code changes in this pull request correctly implement the padding property for the Checkbox widget. The implementation properly handles the resolvedPadding against the ambient Directionality, updates the _CheckboxPainter to use the padding for the check mark and indeterminate dash, and includes comprehensive tests covering edge cases and theme interactions. The documentation also correctly addresses the platform-specific limitation for Checkbox.adaptive on iOS and macOS. The redundant line in debugFillProperties has been removed as requested.
| /// {@endtemplate} | ||
| final String? semanticLabel; | ||
|
|
||
| /// The padding around the check mark when the checkbox is checked or |
There was a problem hiding this comment.
To support global styling and consistent theming across the application, the padding property should also be added to CheckboxThemeData in checkbox_theme.dart and resolved here (e.g., widget.padding ?? checkboxTheme.padding ?? EdgeInsets.zero).
| /// Defaults to [EdgeInsets.zero], which renders the check mark at its full | ||
| /// size (the same as before this property existed). | ||
| final EdgeInsetsGeometry? padding; |
There was a problem hiding this comment.
The documentation states Defaults to [EdgeInsets.zero], but the actual property value defaults to null (which is then resolved to EdgeInsets.zero internally). To be precise and follow Flutter's documentation standards, please clarify this.
For example:
Defaults to null, which resolves to [EdgeInsets.zero].
/// Defaults to null, which resolves to [EdgeInsets.zero], rendering the
/// check mark at its full size (the same as before this property existed).
final EdgeInsetsGeometry? padding;3ffc553 to
7684fd3
Compare
This PR adds an optional padding property to the Checkbox widget.
Currently the check mark always fills the entire checkbox box, with no way to add space between the mark and the box edges (see #188031). This adds a padding field (EdgeInsetsGeometry?) that insets the check mark — and the indeterminate dash — inside the box.
Fixes
Fixes flutter/flutter#188031
Tests
Added to test/material/checkbox_test.dart:
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.