Allows SwitchListTile, CheckboxListTile and RadioListTile to also be able to change horizontalTitleGap, minVerticalPadding, minLeadingWidth #83902
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
Hey @darrenaustin hopefully you don't mind having a look at this? Since you approved the other PR i'm making a reference to. |
HansMuller
left a comment
There was a problem hiding this comment.
Sorry this has taken so long to review.
The changes look reasonable however you'll need to include tests that verify that setting the new constructor parameters have the expected effect.
| contentPadding: contentPadding, | ||
| enabled: onChanged != null, | ||
| onTap: onChanged != null ? () { onChanged!(!value); } : null, | ||
| onTap: onChanged != null |
There was a problem hiding this comment.
Please don't reformat this
| /// If non-null, defines the background color when [SwitchListTile.selected] is true. | ||
| final Color? selectedTileColor; | ||
|
|
||
| /// The horizontal gap between the titles and the leading/trailing widgets. |
There was a problem hiding this comment.
Since this is just a duplicate of the corresponding property from ListTile, change ListTile's API doc by wrapping with
/// {@template flutter.material.ListTile.horizontalTitleGap}
/// - doc goes here -
/// {@endtemplate}
And then here you can include a copy of the same doc with
```dart
/// {@macro flutter.material.ListTile.horizontalTitleGap}Likewise for minVerticalPadding and minLeadingWidth.
| onChanged!(value); | ||
| } | ||
| } : null, | ||
| onTap: onChanged != null |
There was a problem hiding this comment.
Please don't reformat this
| /// If non-null, defines the background color when [RadioListTile.selected] is true. | ||
| final Color? selectedTileColor; | ||
|
|
||
| /// The horizontal gap between the titles and the leading/trailing widgets. |
There was a problem hiding this comment.
See below about using @template and @macro
| /// If non-null, defines the background color when [CheckboxListTile.selected] is true. | ||
| final Color? selectedTileColor; | ||
|
|
||
| /// The horizontal gap between the titles and the leading/trailing widgets. |
There was a problem hiding this comment.
See below about using @template and @macro
|
This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
This PR will need tests before it can land. I'm going to close it for now but please consider opening a new PR if you are able to add tests. Thanks! |
…dioListTile (#178098) <!-- Thanks for filing a pull request! Reviewers are typically assigned within a week of filing a request. To learn more about code review, see our documentation on Tree Hygiene: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md --> This PR adds extra properties to `CheckboxListTile`, `SwitchListTile` and `RadioListTile`. These properties are already implemented for the `ListTile` and passed through from `CheckboxListTile`, `SwitchListTile` and `RadioListTile` to the underlying `ListTile`. The following properties are added: - `horizontalTitleGap` - `minVerticalPadding` - `minLeadingWidth` - `minTileHeight` Fixes #83898 and builds on #154417 and #83902 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot 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. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Victor Sanni <victorsanniay@gmail.com> Co-authored-by: Tong Mu <dkwingsmt@users.noreply.github.com>
Allow SwitchListTile, CheckboxListTile and RadioListTile to also be able to change horizontalTitleGap, minVerticalPadding, minLeadingWidth like here: #64222
List which issues are fixed by this PR. You must list at least one issue.
Fixes #83898
I'm not sure what kind of tests should be made for this, I'm just passing over the values down to the ListTile, and the ListTile already has tests for these properties...
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.