Skip to content

Conversation

@maheshj01
Copy link
Member

Adds enabled property to CheckboxListTile.

Fixes: #102219

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.

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Apr 21, 2022
Copy link
Contributor

@QuncCccccc QuncCccccc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think maybe double slash // here would be better? Got some hints from Style Guide for Flutter:)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this could be: enabled ?? false : null : onChanged

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review, Probably you meant

enabled ??  false ? null : onChanged

But, this wouldn't work for a case when enabled is true.
When enabled is true onChanged would be null. For this API I think onChanged should be enabled (non null) only when enabled is true.

Copy link
Contributor

@amanv8060 amanv8060 May 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably want to pass null when enabled is false. In this case, this might work:
(enabled ?? true) ? onChanged :null

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NICE

@flutter-dashboard
Copy link

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 package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@maheshj01
Copy link
Member Author

Can someone please check why the docs-linux test is failing ?

dartdoc:stdout: Generating docs for library webdriver.sync_core from package:webdriver/sync_core.dart...
dartdoc:stderr: Found 0 warnings and 1 error.
dartdoc:stdout: Documented 134 public libraries in 1006.9 seconds
dartdoc:stderr:
dartdoc:stderr: dartdoc 5.1.2 (/root/.pub-cache/global_packages/dartdoc/bin/dartdoc.dart-2.18.0-175.0.dev.snapshot) failed: encountered 1 errors.

@maheshj01 maheshj01 requested a review from HansMuller June 13, 2022 04:37
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

final Widget control = Checkbox(
value: value,
onChanged: onChanged,
onChanged: enabled ?? true ? onChanged : null ,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NICE

@fluttergithubbot fluttergithubbot merged commit 38df107 into flutter:master Jun 13, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 14, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jun 14, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 14, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 14, 2022
camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 30, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature Request] Provide enabled to CheckboxListTile

5 participants