-
Notifications
You must be signed in to change notification settings - Fork 29.7k
feat: Added docstring examples to AnimatedBuilder and ChangeNotifier #98628
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
feat: Added docstring examples to AnimatedBuilder and ChangeNotifier #98628
Conversation
|
@Hixie I have only updated the documentation so no tests are required |
|
haven't looked at this closely yet, but looks like the analyzer is unhappy. Can you take a look? |
|
Thank you @goderbauer for your time, I've created and tested an example as you suggested |
|
Looks like the checks are unhappy with this change. Can you take a look? |
examples/api/lib/foundation/change_notifier/change_notifier.0.dart
Outdated
Show resolved
Hide resolved
examples/api/lib/foundation/change_notifier/change_notifier.0.dart
Outdated
Show resolved
Hide resolved
examples/api/lib/foundation/change_notifier/change_notifier.0.dart
Outdated
Show resolved
Hide resolved
examples/api/lib/foundation/change_notifier/change_notifier.0.dart
Outdated
Show resolved
Hide resolved
examples/api/lib/foundation/change_notifier/change_notifier.0.dart
Outdated
Show resolved
Hide resolved
examples/api/lib/foundation/change_notifier/change_notifier.0.dart
Outdated
Show resolved
Hide resolved
examples/api/test/foundation/change_notifier/change_notifier.0_test.dart
Outdated
Show resolved
Hide resolved
|
Overall I am actually not convinced that this needs a huge example. Maybe a simple sentence in the doc would be enough. Something like: "Oh, by the way, an AnimiatedBuilder accepts any Listenable (not just an Animation) and works great with Listenable subclasses such as ChangeNotifier, ValueNotifier, etc." (of course phrased a little more formal) That may be easier to understand than having to parse a big block of sample code. |
|
@goderbauer I have simplified the text and also simplified the example. I think that having a short example (along with some descriptive text) is a good thing. A person can play with the code, see it in action and we can make sure that our words are misunderstood 🙂 I've followed you valuable comments to improve the code. I've also removed the |
|
I have thought about this some more and I still think what I said in #98628 (comment) is valid. A simple doc comment saying that any subclass of a Listenable (e.g. ChangeNotifier and friends) can be used with AnimatedBuilder is clearer and more concise than this example. You have to do a lot more reading to get that point from this example - and may even miss what the example is trying to illustrate altogether. |
|
Would you suggest to simply drop the example and keep these lines? /// A [State.setState] method call requires Flutter to entirely rebuild the
/// current widget plus a variable portion of the subtree, depending on the
/// depth and the structure.
///
/// To avoid unnecessary work, the [AnimatedBuilder] class can be used along
/// with a [Listenable] to only rebuild certain portions a widget,
/// leaving other parts and the subtree untouched. For example,
/// [ChangeNotifier] and [ValueNotifier] are subtypes of [Listenable].
///
/// Since a [ChangeNotifier] is a subtype of [Listenable], it can be used in
/// an [AnimatedBuilder] to listen for changes.I would really like to keep the example but we can remove it if you want 🙂 I think that we could keep the lines I posted above. If we wrote even less contents, maybe it wouldn't give enough context to understand why one should use a ChangeNotifier in an AnimatedBuilder. Also, a very few lines wouldn't add much value and so they wouldn't even need to be there. What do you think? Do you agree in at least keeping that text? |
|
Here's my suggestion for the docs: If you want to include an example below that, let's use one that is concise and focused on that particular use case. Here's my suggestion: import 'package:flutter/material.dart';
class CounterBody extends StatelessWidget {
const CounterBody({Key? key, required this.counterValueNotifier}) : super(key: key);
final ValueNotifier<int> counterValueNotifier;
@override
Widget build(BuildContext context) {
return Center(
child: Column(
mainAxisAlignment: MainAxisAlignment.center,
children: [
const Text('Current counter value:'),
// Thanks to the [AnimatedBuilder], only the widget displaying the
// current count is rebuilt when `counterValueNotifier` notifies its
// listeners.
AnimatedBuilder(
// [AnimatedBuilder] accepts any [Listenable] subtype.
animation: counterValueNotifier,
builder: (BuildContext context, Widget? child) {
return Text('${counterValueNotifier.value}');
},
),
],
),
);
}
}
class MyApp extends StatefulWidget {
const MyApp({Key? key}) : super(key: key);
@override
State<MyApp> createState() => _MyAppState();
}
class _MyAppState extends State<MyApp> {
final ValueNotifier<int> _counter = ValueNotifier<int>(0);
@override
Widget build(BuildContext context) {
return MaterialApp(
home: Scaffold(
appBar: AppBar(title: const Text('AnimatedBuilder example')),
body: CounterBody(counterValueNotifier: _counter),
floatingActionButton: FloatingActionButton(
onPressed: () => _counter.value++,
child: const Icon(Icons.add),
),
),
);
}
}
void main() {
runApp(const MyApp());
} |
|
I really like your changes! Thank you for having considered keeping an example, I think it will be very valuable |
goderbauer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Please rebase it to the latest master, though.
…notifier_doc_update
|
I have rebased but the diff isn't going away... |
goderbauer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Piinks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.

I have added an example to document how an
AnimatedBuildercan be used along with aChangeNotifier. Follow up for #97256List which issues are fixed by this PR. You must list at least one issue.
Pre-launch Checklist
///).