[cupertino_ui] Migrate api sample code to @example dartdoc directive#12063
Conversation
There was a problem hiding this comment.
Code Review
This pull request replaces {@tool dartpad} and {@tool snippet} blocks with {@example} directives across various Cupertino widgets, adds temporary TODO comments, and configures analysis_options.yaml to ignore doc_directive_unknown warnings. The feedback points out that using // instead of /// for the TODO comments breaks the contiguous doc comment blocks, which will cause the main widget descriptions to be omitted from the generated API documentation. Additionally, the reviewer notes that the paths in the {@example} directives should be relative to the package root by removing the leading slash.
| // TODO(dkwingsmt): Replace the following block with a blue example container | ||
| // when it's supported. https://github.com/dart-lang/dartdoc/issues/4243 | ||
| /// | ||
| /// This example shows how [CupertinoActivityIndicator] can be customized. | ||
| /// | ||
| /// ** See code in examples/api/lib/cupertino/activity_indicator/cupertino_activity_indicator.0.dart ** | ||
| /// {@end-tool} | ||
| /// {@example /example/lib/activity_indicator/cupertino_activity_indicator.0.dart} | ||
| /// | ||
| // TODO(dkwingsmt): End of the blue example container. |
There was a problem hiding this comment.
In Dart, any line starting with // (instead of ///) breaks the contiguous block of doc comments. This causes the doc comment block before the // TODO to be detached from the class/member declaration, meaning the main description of the widget/class will be completely missing from the generated API documentation.
To prevent this, use /// for the TODO comments to keep the doc comment block contiguous. Additionally, the path in {@example} should be relative to the package root (e.g., example/lib/... instead of /example/lib/...), as a leading slash can cause resolution issues. This issue is present across almost all files in this PR and should be fixed everywhere.
| // TODO(dkwingsmt): Replace the following block with a blue example container | |
| // when it's supported. https://github.com/dart-lang/dartdoc/issues/4243 | |
| /// | |
| /// This example shows how [CupertinoActivityIndicator] can be customized. | |
| /// | |
| /// ** See code in examples/api/lib/cupertino/activity_indicator/cupertino_activity_indicator.0.dart ** | |
| /// {@end-tool} | |
| /// {@example /example/lib/activity_indicator/cupertino_activity_indicator.0.dart} | |
| /// | |
| // TODO(dkwingsmt): End of the blue example container. | |
| /// TODO(dkwingsmt): Replace the following block with a blue example container | |
| /// when it's supported. https://github.com/dart-lang/dartdoc/issues/4243 | |
| /// | |
| /// This example shows how [CupertinoActivityIndicator] can be customized. | |
| /// | |
| /// {@example example/lib/activity_indicator/cupertino_activity_indicator.0.dart} | |
| /// | |
| /// TODO(dkwingsmt): End of the blue example container. |
There was a problem hiding this comment.
It might be, but I think we want that anyways. There will be no blue box initially so the line break will distinguish the separation of the code block from the rest of the docs?
| // TODO(dkwingsmt): Replace the following block with a blue example container | ||
| // when it's supported. https://github.com/dart-lang/dartdoc/issues/4243 | ||
| /// | ||
| /// This example shows how [CupertinoLinearActivityIndicator] can be customized. | ||
| /// | ||
| /// ** See code in examples/api/lib/cupertino/activity_indicator/cupertino_linear_activity_indicator.0.dart ** | ||
| /// {@end-tool} | ||
| /// {@example /example/lib/activity_indicator/cupertino_linear_activity_indicator.0.dart} | ||
| /// | ||
| // TODO(dkwingsmt): End of the blue example container. |
There was a problem hiding this comment.
Using // breaks the contiguous doc comment block, detaching the main description of CupertinoLinearActivityIndicator from its declaration. Use /// instead to keep the block contiguous, and remove the leading slash from the {@example} path.
| // TODO(dkwingsmt): Replace the following block with a blue example container | |
| // when it's supported. https://github.com/dart-lang/dartdoc/issues/4243 | |
| /// | |
| /// This example shows how [CupertinoLinearActivityIndicator] can be customized. | |
| /// | |
| /// ** See code in examples/api/lib/cupertino/activity_indicator/cupertino_linear_activity_indicator.0.dart ** | |
| /// {@end-tool} | |
| /// {@example /example/lib/activity_indicator/cupertino_linear_activity_indicator.0.dart} | |
| /// | |
| // TODO(dkwingsmt): End of the blue example container. | |
| /// TODO(dkwingsmt): Replace the following block with a blue example container | |
| /// when it's supported. https://github.com/dart-lang/dartdoc/issues/4243 | |
| /// | |
| /// This example shows how [CupertinoLinearActivityIndicator] can be customized. | |
| /// | |
| /// {@example example/lib/activity_indicator/cupertino_linear_activity_indicator.0.dart} | |
| /// | |
| /// TODO(dkwingsmt): End of the blue example container. |
| // TODO(dkwingsmt): Replace the following block with a blue example container | ||
| // when it's supported. https://github.com/dart-lang/dartdoc/issues/4243 | ||
| // TODO(dkwingsmt): Add unit tests to this code snippet. | ||
| // https://github.com/flutter/flutter/issues/188530 |
There was a problem hiding this comment.
Using // breaks the contiguous doc comment block, detaching the main description of CupertinoApp from its declaration. Use /// instead to keep the block contiguous.
/// TODO(dkwingsmt): Replace the following block with a blue example container
/// when it's supported. https://github.com/dart-lang/dartdoc/issues/4243
/// TODO(dkwingsmt): Add unit tests to this code snippet.
/// https://github.com/flutter/flutter/issues/188530| // TODO(dkwingsmt): End of the blue example container. | ||
| /// | ||
| // TODO(dkwingsmt): Replace the following block with a blue example container | ||
| // when it's supported. https://github.com/dart-lang/dartdoc/issues/4243 | ||
| // TODO(dkwingsmt): Add unit tests to this code snippet. | ||
| // https://github.com/flutter/flutter/issues/188530 |
There was a problem hiding this comment.
Using // breaks the contiguous doc comment block. Use /// instead to keep the block contiguous.
/// TODO(dkwingsmt): End of the blue example container.
///
/// TODO(dkwingsmt): Replace the following block with a blue example container
/// when it's supported. https://github.com/dart-lang/dartdoc/issues/4243
/// TODO(dkwingsmt): Add unit tests to this code snippet.
/// https://github.com/flutter/flutter/issues/188530| // TODO(dkwingsmt): Replace the following block with a blue example container | ||
| // when it's supported. https://github.com/dart-lang/dartdoc/issues/4243 | ||
| /// | ||
| /// This example shows a [CupertinoTabBar] placed in a [CupertinoTabScaffold]. | ||
| /// | ||
| /// ** See code in examples/api/lib/cupertino/bottom_tab_bar/cupertino_tab_bar.0.dart ** | ||
| /// {@end-tool} | ||
| /// {@example /example/lib/bottom_tab_bar/cupertino_tab_bar.0.dart} | ||
| /// | ||
| // TODO(dkwingsmt): End of the blue example container. |
There was a problem hiding this comment.
Using // breaks the contiguous doc comment block, detaching the main description of CupertinoTabBar from its declaration. Use /// instead to keep the block contiguous, and remove the leading slash from the {@example} path.
/// TODO(dkwingsmt): Replace the following block with a blue example container
/// when it's supported. https://github.com/dart-lang/dartdoc/issues/4243
///
/// This example shows a [CupertinoTabBar] placed in a [CupertinoTabScaffold].
///
/// {@example example/lib/bottom_tab_bar/cupertino_tab_bar.0.dart}
///
/// TODO(dkwingsmt): End of the blue example container.| // TODO(dkwingsmt): Replace the following block with a blue example container | ||
| // when it's supported. https://github.com/dart-lang/dartdoc/issues/4243 | ||
| /// | ||
| /// This sample shows produces an enabled and disabled [CupertinoButton] and | ||
| /// [CupertinoButton.filled]. | ||
| /// | ||
| /// ** See code in examples/api/lib/cupertino/button/cupertino_button.0.dart ** | ||
| /// {@end-tool} | ||
| /// {@example /example/lib/button/cupertino_button.0.dart} | ||
| /// | ||
| // TODO(dkwingsmt): End of the blue example container. |
There was a problem hiding this comment.
Using // breaks the contiguous doc comment block, detaching the main description of CupertinoButton from its declaration. Use /// instead to keep the block contiguous, and remove the leading slash from the {@example} path.
/// TODO(dkwingsmt): Replace the following block with a blue example container
/// when it's supported. https://github.com/dart-lang/dartdoc/issues/4243
///
/// This sample shows produces an enabled and disabled [CupertinoButton] and
/// [CupertinoButton.filled].
///
/// {@example example/lib/button/cupertino_button.0.dart}
///
/// TODO(dkwingsmt): End of the blue example container.
justinmc
left a comment
There was a problem hiding this comment.
Looks good minus my comment about distinguishing dartpad blocks. And I think Gemini is wrong about its double slash concern?
| /// {@tool dartpad} | ||
| // TODO(dkwingsmt): Replace the following block with a blue example container | ||
| // when it's supported. https://github.com/dart-lang/dartdoc/issues/4243 |
There was a problem hiding this comment.
I think we need a TODO that's different for dartpad vs. snippet, so that we can distinguish them later when we put the dartpads back. See discussion here: #11975 (comment)
There was a problem hiding this comment.
Oh good call! Let me update marking which ones were dartpads. 👍
| // TODO(dkwingsmt): Replace the following block with a blue example container | ||
| // when it's supported. https://github.com/dart-lang/dartdoc/issues/4243 | ||
| /// | ||
| /// This example shows how [CupertinoActivityIndicator] can be customized. | ||
| /// | ||
| /// ** See code in examples/api/lib/cupertino/activity_indicator/cupertino_activity_indicator.0.dart ** | ||
| /// {@end-tool} | ||
| /// {@example /example/lib/activity_indicator/cupertino_activity_indicator.0.dart} | ||
| /// | ||
| // TODO(dkwingsmt): End of the blue example container. |
There was a problem hiding this comment.
LGTM except for some minor places. Thank you Kate for working on this!
On a stricter note: While the current resolution for dartpads are
// TODO: Migrate the following block to dartpad
/// ...
// TODO: end of dartpad
My guess is that the following way is closer to the eventual format, hence slightly easier for the future migration:
// TODO: Start of blue container
/// ...
// TODO: Migrate the following directive to dartpad
/// ...
// TODO: end of blue container
However, this is no big deal. It's just a global find-and-replace away and we can still do it when we actually migrate to the blue containers. We should proceed with the current way to unblock the first release.
| // `_SlideTarget`s. | ||
| // | ||
| // TODO(dkwingsmt): It should recompute hit testing when the app is updated, | ||
| // TODO(framework): It should recompute hit testing when the app is updated, |
There was a problem hiding this comment.
Unintentional change? Reverting:
| // TODO(framework): It should recompute hit testing when the app is updated, | |
| // TODO(dkwingsmt): It should recompute hit testing when the app is updated, |
There was a problem hiding this comment.
Oh ok! I guess I felt odd making all these TODOs yours. :)
| /// | ||
| /// This sample demonstrates how to use [CupertinoTextMagnifier]. | ||
| /// | ||
| /// ** See code in examples/api/lib/widgets/magnifier/cupertino_text_magnifier.0.dart ** |
There was a problem hiding this comment.
This is a widgets example, I think I need to bring this sample code over. FYI @dkwingsmt we should check that there aren't samples from the widgets/* or other libraries in the material_ui version of this.
| /// | ||
| /// This sample demonstrates how to use [CupertinoMagnifier]. | ||
| /// | ||
| /// ** See code in examples/api/lib/widgets/magnifier/cupertino_magnifier.0.dart ** |
| /// | ||
| /// This sample demonstrates how to customize the magnifier that this text field uses. | ||
| /// | ||
| /// ** See code in examples/api/lib/widgets/text_magnifier/text_magnifier.0.dart ** |
|
Added missing samples in #12086. I'll push a PR in flutter/flutter as well to keep sync. |
|
@Piinks Can you address #12063 (review) ? |
Yup, I already did, I just haven't pushed it yet because I need to add the missing samples as well from #12086 |
|
Which just merged! 🎊 Updating now |
dkwingsmt
left a comment
There was a problem hiding this comment.
Also, menu_anchor.dart contains the only two instances of @tool sample. I've linked how they should be converted.
| /// [CupertinoColors.systemRed]. | ||
| /// | ||
| /// {@tool sample} | ||
| // TODO(framework): Replace the following block with a @dartpad directive |
There was a problem hiding this comment.
This is a @tool sample, not a dartpad.
Here's how I think it should be converted:
https://github.com/flutter/packages/pull/11975/changes#diff-d3032846508ffdddebef87d9bbdb1fe55360296bbd1b2a010d1de6e8ded91cc8R298
| /// | ||
| /// ## Usage | ||
| /// {@tool sample} | ||
| // TODO(framework): Replace the following block with a @dartpad directive |
There was a problem hiding this comment.
This is a @tool sample, not a dartpad.
Here's how I think it should be converted:
https://github.com/flutter/packages/pull/11975/changes#diff-d3032846508ffdddebef87d9bbdb1fe55360296bbd1b2a010d1de6e8ded91cc8R298
|
Ah shoot, I think when I did a find and replace to update the dart issue links, I replaced ones I did not mean to. |
I am pretty sure I am right. Updating. |
Yes, the issues are right. |
| /// | ||
| /// {@tool snippet} | ||
| // TODO(framework): Replace the following block with a blue example container | ||
| // when it's supported. https://github.com/dart-lang/dartdoc/issues/4123 |
There was a problem hiding this comment.
4123 is the dartpad issue :(
There was a problem hiding this comment.
Yup, as I said, I noticed some were updated incorrectly when I did a find and replace. Just pushed a new commit
…12078) Migrates @tool directives to @example in material_ui Towards flutter/flutter#172936 and flutter/flutter#172937 Prerequisite: * #12092, which moves missing sample files. Related: * #12063, which migrates `cupertino_ui` ## Pre-Review Checklist **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. [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.
Migrates
@tooldirectives to@examplein cupertino_uiTowards flutter/flutter#172936 and flutter/flutter#172937
Referenced: https://github.com/flutter/packages/pull/11975/files
Pre-Review Checklist
[shared_preferences]///).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. 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.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2