Skip to content

Conversation

@legalcodes
Copy link
Contributor

@legalcodes legalcodes commented Oct 16, 2019

@googlebot googlebot added the cla: yes Contributor has signed the Contributor License Agreement label Oct 16, 2019
@legalcodes legalcodes requested a review from sfshaza2 October 21, 2019 16:50
Copy link
Contributor

@sfshaza2 sfshaza2 left a comment

Choose a reason for hiding this comment

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

@legalcodes, I defer to others on reviewing the code and I have some Grammar Police nits but, overall, I think this is a great codelab! I really like how you've explained things and how you've used DP.

Once the nits are addressed, I say LGTM..

@@ -0,0 +1,432 @@
---
title: "Implicit Animations"
Copy link
Contributor

Choose a reason for hiding this comment

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

Sentence case: "Implicit animations"

- One "Show Details" button that does nothing when clicked.
- Description text of the owl in the photograph.

Run the example to render the following code:
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe more direct?
"Click the Run button to run the example:"

I clicked the Run button over and over. It merely says "no issues". I get no output. I also tried "Reset" and then "Run". I tried multiple times. No output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per our discussion, I've raised dart-lang/dart-pad#1340

Could you add a screenshot and any additional description you might have of this issue?

Many thanks!

Copy link
Member

Choose a reason for hiding this comment

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

For me, dartpad was working fine.

```

{{site.alert.secondary}}
Notice that you only need to set the beginning and ending values of `opacity`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is nitpicky, but we indent alerts by 2 spaces per line.

The `AnimatedOpacity` widget manages everything in between.
{{site.alert.end}}

Here's the example with the completed changes you've just made -- run this
Copy link
Contributor

@sfshaza2 sfshaza2 Oct 21, 2019

Choose a reason for hiding this comment

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

GLOBAL: use an — for a long dash, or – for a short dash. This would use a long dash. Also, don't bracket it with spaces: https://developers.google.com/style/dashes

{{site.alert.end}}

Here's the example with the completed changes you've just made -- run this
example and click on the "Show Details" button to trigger the animation.
Copy link
Contributor

@sfshaza2 sfshaza2 Oct 21, 2019

Choose a reason for hiding this comment

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

It's "click" not "click on". (Search for click in the word list: https://developers.google.com/style/word-list)

Also, UI is denoted in bold, not quotes, so "Show Details: https://developers.google.com/style/ui-elements
And, if I wanted to be extra nitpicky, I'd suggest Show details.

By the way, I still can't run the example.

`borderRadius`, and `margin`) has an associated function (`randomColor`,
`randomBorderRadius`, and `randomMargin` respectively) that returns a randomly
generated value for that property. By using an `AniamtedContainer` widget, you can
quickly refactor this code to:
Copy link
Contributor

Choose a reason for hiding this comment

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

Sentence fragment. => "refactor this code to do the following:"

),
```

**5.** Finally, set the duration of the animation that will power the transition
Copy link
Contributor

Choose a reason for hiding this comment

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

"that will power" => "that powers"

color: Theme.of(context).primaryColor,
```

Here’s the example with the completed changes you’ve just made – run the code
Copy link
Contributor

@sfshaza2 sfshaza2 Oct 21, 2019

Choose a reason for hiding this comment

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

— for an —


The preceding example demonstrates animated transitions between values for
the container's `margin`, `borderRadius`, and `color` properties. However,
`AnimatedContainer` animates changes in all of its properties, including ones
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a fan of "ones":
"including ones..." => including those that...

[linear animation curve] by default.

However, watch how the animation changes in the preceding example
when you pass a `fastOutSlowIn` curve:
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmmm... I'm of two minds about this. When it says "watch how the animation changes..." I'm expecting another DartPad. But, as you are probably thinking, do we NEED another DP here? I think not, but I'd rephrase this to say something like, "make the following change to the previous DartPad to see how the..."

Copy link
Contributor Author

@legalcodes legalcodes Oct 21, 2019

Choose a reason for hiding this comment

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

This was bothering me, you offered the perfect phrase for the job. Many thanks Shams!

Copy link
Member

Choose a reason for hiding this comment

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

Lol, I also was looking for another dartpad here... But yeah, telling people to modify the previous one sounds good.

{{site.alert.secondary}}
This section contains a list of steps you can use to add an implicit animation to the
preceding example code. After the steps, you can also run an example with the
the changes already made.
Copy link
Member

Choose a reason for hiding this comment

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

remove the "the"? It's already on the preceding line.

that manage animations for you. These widgets are collectively referred to as _implicit
animations_, or _implicitly animated widgets_, deriving their name from the
[ImplicitlyAnimatedWidget] class that they implement. Implicit animations trade
control for convenience—they manage animation effects so that you don't have to.
Copy link
Member

Choose a reason for hiding this comment

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

I always like to say that with implicit animations you you set a target value and when that target value changes the widget animates you from the old to the new.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perfect! This adds some much-needed glue the intro was missing. Many thanks.

The previous example also uses a common workflow for using implicit animations:
- First, pick a widget property to animate.
- Next, choose an implicit animation that can animate that property.
- Set the start and end values for the property that you're animating.
Copy link
Member

Choose a reason for hiding this comment

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

This sounds kinda odd. You're really only picking a start value. In the last bullet point, where you trigger the animation you basically trigger it by picking the end value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think you're right -- Set the start and end values is misleading. The "workflow" I'm summarizing here is essentially a series of decisions made at design time, so Choose the start and end values... would be the better phrase. WDYT?

import 'package:flutter_web_test/flutter_web_test.dart';
import 'package:flutter_web_ui/ui.dart' as ui;
import 'dart:math';
const _duration = Duration(milliseconds: 400);
Copy link
Member

Choose a reason for hiding this comment

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

nit: add an empty line above this line.

@@ -0,0 +1,67 @@
import 'package:flutter_web/material.dart';
Copy link
Member

Choose a reason for hiding this comment

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

Do you need license headers for all the .dart files?

double borderRadius;
double margin;

initState() {
Copy link
Member

Choose a reason for hiding this comment

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

@override
void initState() {
  super.initState();

double margin;

initState() {
final color = randomColor();
Copy link
Member

Choose a reason for hiding this comment

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

remove the "final" on these lines? Not sure what that even means...

Text('Age: 39'),
Text('Employment: None'),
],
))
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
))
),
),

import 'package:flutter_web_test/flutter_web_test.dart';
import 'package:flutter_web_ui/ui.dart' as ui;

const owl_url =
Copy link
Member

Choose a reason for hiding this comment

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

nit: I would just leave this on one line (unless you are bound to 80 chars per line)

Widget build(BuildContext context) {
return MaterialApp(
home: Scaffold(
body: Center(child: FadeInDemo()),
Copy link
Member

Choose a reason for hiding this comment

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

Indentation is off here. This should probably be:

    return MaterialApp(
      home: Scaffold(
        body: Center(child: FadeInDemo()),
      ),
    );


The previous example also uses a common workflow for using implicit animations:
- First, pick a widget property to animate.
- Next, choose an implicit animation that can animate that property.
Copy link
Contributor

Choose a reason for hiding this comment

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

Might want to say somewhere that you are choosing a subtype of ImplicitlyAnimatedWidget

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure -- how about something like:

Remember, an implicit animation is a subtype of `ImplicitlyAnimatedWidget`, and you can
browse the [list of common implicitly animated widgets] that ship with Flutter.


- A shape with size, margin, and color that are different each time you run the
example.
- A **Change** button that does nothing when clicked.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to include the StatelessWidget to StatefulWidget change? In this case, why not have the sample start with a StatefulWidget that changes the shape and color, and then having the user make it animated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's followup to make sure I've fully captured this suggestion in the next change.

@galeyang
Copy link
Contributor

Like

  • Overall, it’s very clear on what the step is about, what code to change
  • Like clear expectations: prerequirest, covered topics, est time
  • Static code differences are very clear
  • Like the animated curve graph

Potential improvements

  • I kind of wish that I see this part earlier. I tried to summarize the high-level flow when I was reading the detailed steps. This part gives me a very clear overview.
    image

  • I’m not sure if we should use auto-run in some of reading-centric examples. What value do users get from spending effort to click the run button?

  • Trivial: Would be helpful to remind me where to add the code. The preceding examples do not specify a curve, so the implicit animations apply a linear animation curve by default. Add a curve parameter to the preceding example and watch how the animation changes when you pass the easeInOutBack constant for curve:

  • Nit: text said "Show details" but the code is “Show Details”

Wishes
Some notes are outside of your scope but are general notes that we can improve in other PRs)

  • I wish that I can copy code snippets in those code difference blocks more easily
    • When I wanted to practice these steps, I tried to copy the snippet to the DartPad above but it’s hard to do that. Issues (1) got irrelevant syntax (e.g. the plug sign) (2) Lots of scrolling. Eventually, I decided to read through the code
  • (Trivial) Wish that the different code is highlighted in DartPad
  • (Trivial) It would be good that we sometimes use Cupertino style (in new codelabs)
  • (Trivial) For DartPad, I wish that there is a small hint to let me know the output is not recompiled based on my updated code yet.

General question: what’s the difference between codelab and cookbook?

@legalcodes
Copy link
Contributor Author

legalcodes commented Oct 25, 2019

Awesome @galeyang, per our discussion I plan to make the following changes:

  • Distribute the "workflow" summary points throughout each step
  • Wait for a later PR to use auto-run if we find it necessary
  • Use named-examples with links, and also provide a hint to the user to use the provided line numbers for reference as the user makes suggested changes
  • Fix the nit

@legalcodes legalcodes changed the title [WIP] Add implicit animations codelab Add implicit animations codelab Oct 29, 2019
@sfshaza2
Copy link
Contributor

LGTM

@sfshaza2 sfshaza2 merged commit 2714d9f into flutter:master Oct 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Contributor has signed the Contributor License Agreement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants