Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Doc Samples For CheckboxListTile, RadioListTile and SwitchListTile #32703

Merged
merged 35 commits into from May 17, 2019

Conversation

@shihaohong
Copy link
Contributor

commented May 14, 2019

Description

Adds samples to ListTile variants so that developers have some structure to go off of if they decide to create their own custom [Checkbox/Radio/Switch]Label widgets. The goal is to emphasize that ListTiles aren't a one-size-fits-all solution and that it's possible to create a custom widget that better suits developers' needs.

Related Issues

Fixes #30781

Tests

No tests since these are code samples in the API documentation.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
  • No, this is not a breaking change.

shihaohong added some commits May 14, 2019

@shihaohong shihaohong requested a review from HansMuller May 14, 2019

@googlebot googlebot added the cla: yes label May 14, 2019

@shihaohong shihaohong changed the title Added Doc Samples For Checkbox/Radio/Switch with a Label Add Doc Samples For Checkbox/Radio/Switch with a Label May 14, 2019

shihaohong added some commits May 14, 2019

@HansMuller
Copy link
Contributor

left a comment

LGTM

@shihaohong shihaohong requested review from HansMuller and chunhtai May 14, 2019

@shihaohong

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

Made some minor updates to incorporate @chunhtai's example with RichText to teach developers to avoid #31437. Please review, thanks!

@shihaohong

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

The analyzer test is failing because there's a bug in the API doc sample generator that does not properly insert import 'packages:flutter/gestures.dart for the app sample. I filed an issue to track it: #32725

@chunhtai

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

LGTM

@shihaohong

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

Decided offline with @HansMuller to create two separate samples: one to address the Semantics example and one to address the [Switch/Radio/Checkbox] + Label sample issue. That way, there's less confusion from trying to combine the two takeaways into one sample.

@HansMuller
Copy link
Contributor

left a comment

This is turning into a nice big PR!

Feedback is just a few minor wording suggestions. Most of them should apply to all of the samples.

/// Here is an example of a custom LinkedLabelCheckbox widget that allows a
/// [RichText] widget to be nested in a checkbox tile widget.
///
/// Since CheckboxListTile is wrapped with a [MergeSemantics] widget, it wants

This comment has been minimized.

Copy link
@HansMuller

HansMuller May 15, 2019

Contributor

I think the issue is that we're including a RichText widget that has an embedded gesture recognizer.

CheckboxListTile wraps its children with a [MergeSemantics] widget so they will appear as just one interactive node in the semantics tree. The LinkedLabelCheckbox cannot be merged in that way because it contains two interactive element, its checkbox and its text.

This comment has been minimized.

Copy link
@shihaohong

shihaohong May 16, 2019

Author Contributor

I updated it to be the following:

/// Since CheckboxListTile wraps its children with a [MergeSemantics] widget, it
/// wants to merge its descendant [Semantics] nodes to appear as just one
/// interactive node in the semantics tree.
///
/// Thus, CheckboxListTile will not work with children widgets that need to be \
/// a standalone interactive element. One example would be the [RichText]
/// widget, which has an embedded gesture recognizer configured to have
/// its own [Semantics] node. Thus, you may need to create a custom widget to
/// accommodate your needs.

@shihaohong shihaohong requested review from chunhtai and HansMuller May 16, 2019

@shihaohong shihaohong changed the title Add Doc Samples For Checkbox/Radio/Switch with a Label Add Doc Samples For CheckboxListTile, RadioListTile and SwitchListTile May 16, 2019

@shihaohong

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

Did the following:

  1. Added in image examples
  2. Made the first sample on each page an entire app to help with copy/pasting to test
  3. Fixed up semantics paragraphs to have a cleaner description

@shihaohong shihaohong dismissed stale reviews from chunhtai and HansMuller May 16, 2019

Stale review

@HansMuller
Copy link
Contributor

left a comment

LGTM

/// nodes into one node in the semantics tree. Therefore, CheckboxListTile will
/// throw an error if any of its children requires its own [Semantics] node.
///
/// For example, you cannot nest a [RichText] widget as a descendant of

This comment has been minimized.

Copy link
@HansMuller

HansMuller May 17, 2019

Contributor

NICE

@shihaohong shihaohong merged commit a0ed52c into flutter:master May 17, 2019

35 checks passed

WIP Ready for review
Details
add2app-macos Task Summary
Details
add2app-macos
Details
analyze Task Summary
Details
analyze
Details
build_tests-linux Task Summary
Details
build_tests-linux
Details
build_tests-macos Task Summary
Details
build_tests-macos
Details
build_tests-windows Task Summary
Details
build_tests-windows
Details
cla/google All necessary CLAs are signed
deploy_gallery Task Summary
Details
deploy_gallery
Details
deploy_gallery-macos Task Summary
Details
deploy_gallery-macos
Details
docs Task Summary
Details
docs
Details
flutter-build
Details
integration_tests-linux Task Summary
Details
integration_tests-linux
Details
integration_tests-windows Task Summary
Details
integration_tests-windows
Details
tests-linux Task Summary
Details
tests-linux
Details
tests-macos Task Summary
Details
tests-macos
Details
tests-windows Task Summary
Details
tests-windows
Details
tool_tests-linux Task Summary
Details
tool_tests-linux
Details
tool_tests-macos Task Summary
Details
tool_tests-macos
Details
tool_tests-windows Task Summary
Details
tool_tests-windows
Details

@shihaohong shihaohong deleted the shihaohong:label-examples branch May 17, 2019

Kiku-Reise added a commit to Kiku-Reise/flutter that referenced this pull request Jun 14, 2019

Add Doc Samples For CheckboxListTile, RadioListTile and SwitchListTile (
flutter#32703)

* Moved Radio documentation line to be above sample

* Added LabeledRadio sample

* Add LabeledCheckbox sample

* Add LabeledSwitch sample

* Added LinkedLabelRadio sample to RadioListTile

* Added LinkedLabelCheckbox sample to CheckboxListTile

* Added LinkedLabelSwitch sample to SwitchListTile

* Added reference to Semantics docs

* Improve simple SwitchListTile, RadioListTile and CheckboxListTile samples

* Added assets to all SwitchListTile, RadioListTile and CheckboxListTile samples
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.