Conversation
googlebot
added
the
cla: yes
kwalrath
left a comment
kwalrath
left a comment
There was a problem hiding this comment.
Most of the changes look good, but of course, I found nits to pick...
| forcing the use of a particular version. | ||
|
|
||
| Forcing the use of `url_launcher` version `0.4.3` in `counter/pubspec.yaml`: | ||
| To force the use of `url_launcher` version `0.4.3` in the app's `pubspec.yaml`: |
There was a problem hiding this comment.
This should be a complete sentence (https://developers.google.com/style/code-samples?hl=ru#intros). Sentence fragments introducing code samples seem to occur a lot in this page.
|
|
||
| Forcing the use of `guava` version `23.0` in `counter/android/build.gradle`: | ||
| To force the use of `guava` version `23.0` in the app's | ||
| `android/build.gradle` file: |
There was a problem hiding this comment.
This, too, should be a complete sentence.
| * Range constraint with [*caret | ||
| * Range constraints with [*caret | ||
| syntax*]({{site.dart-site}}/tools/pub/dependencies#caret-syntax) | ||
| is similar to regular range constraints: |
|
|
||
| If you want to upgrade to a new version of the package, | ||
| To upgrade to a new version of the package, | ||
| for example to use new features in that package, run |
There was a problem hiding this comment.
For ease of reading, consider using em-dashes or parentheses instead of commas to set off the "for example...".
| Finally, use the `ref` argument to pin the dependency to a | ||
| specific git commit, branch, or tag. For more details, see | ||
| [Pub Dependencies]({{site.dart-site}}/tools/pub/dependencies). | ||
| [Package Dependencies]({{site.dart-site}}/tools/pub/dependencies). |
There was a problem hiding this comment.
Dependencies -> dependencies
| and using third-party platform SDKs (like | ||
| [Firebase]({{site.github}}/flutter/plugins/blob/master/FlutterFire.md)). | ||
| and using third-party platform SDKs | ||
| ([Firebase]({{site.github}}/flutter/plugins/blob/master/FlutterFire.md)). |
There was a problem hiding this comment.
Firebase -> FlutterFire?
If not, then maybe:
SDKs like Firebase (FlutterFire).
| to the Flutter and Dart ecosystems. This allows you to quickly build | ||
| your app without having to develop everything from scratch. | ||
|
|
||
| Existing packages enable many use cases, for example, making network requests |
|
|
||
| If you are looking to add assets, images, or fonts, whether stored in | ||
| files or packages, please see [Assets & images](/docs/development/ui/assets-and-images). | ||
| If you are looking to develop a new package, see [developing |
There was a problem hiding this comment.
GLOBAL:
you are looking to -> you want to
(or "you intend to")
| files or packages, please see [Assets & images](/docs/development/ui/assets-and-images). | ||
| If you are looking to develop a new package, see [developing | ||
| packages](/docs/development/packages-and-plugins/developing-packages). | ||
| If you are looking to add assets, images, or fonts, |
There was a problem hiding this comment.
There are a lot of commas in this sentence. How about:
fonts, -> fonts —
packages, -> — packages
| @@ -207,47 +214,45 @@ additional dependency options are avaialble: | |||
|
|
|||
| * **Git** dependency on a package in a folder: By default Pub assumes the | |||
| package is located in the root of the Git repository. If that is not the | |||
There was a problem hiding this comment.
I think you can reduce the "By default..." sentence and combine it with the following one.
sfshaza2
changed the title
| url: "https://pub.dartlang.org" | ||
| source: hosted | ||
| version: "0.6.1" | ||
| version: "0.5.0" |
There was a problem hiding this comment.
I usually avoid updating pubspec.lock unless I'm sure I want to do that...
kwalrath
left a comment
kwalrath
left a comment
There was a problem hiding this comment.
Here's my review up through src/docs/cookbook/forms/focus.md. The focus doc needs some work, due to its use of "focus on." Everything else looks pretty great! I'm very happy that you're making these fixes, even if it does make for a very long review...
More later.
| DartPad! You might encounter bugs, | ||
| malapropisms, annoyances, and other general weirdness. | ||
| If that happens, please take a moment to | ||
| If that happens, we appreciate if you take a moment to |
There was a problem hiding this comment.
Not sure why "we appreciate if you" is better than "please". Maybe we should settle on some standard wording? In https://dart.dev/codelabs/dart-cheatsheet, we used to have some similar wording. I changed it to the text in the yellow box.
| Use the custom State class to define the properties you need to change over | ||
| Use the custom State class to define the properties that change over | ||
| time. In this example, that includes the width, height, color, and border | ||
| radius. In addition, you can also define the default value of each property. |
There was a problem hiding this comment.
I'd drop "In addition," and change this to "You can also..."
| a smooth experience. | ||
|
|
||
| In Flutter, you can achieve this task using the [`AnimatedOpacity`][] Widget. | ||
| In Flutter, achieve by using the [`AnimatedOpacity`][] widget. |
There was a problem hiding this comment.
There's a missing word or two here. And "Flutter" seems unnecessary. Maybe something like this:
The ... widget makes it easy to perform opacity animations.
OR
This page shows how to use the ... widget to perform opacity animations.
| @@ -112,23 +115,23 @@ FloatingActionButton( | |||
| ## 4. Fade the box in and out | |||
|
|
|||
| You've got a green box on screen. You've got a button to toggle the visibility | |||
There was a problem hiding this comment.
"You've got" rubs me the wrong way (thank you, Dad). How about "You have"?
(I seem to be in the minority: https://www.quickanddirtytips.com/education/grammar/is-have-got-acceptable-english is the only place I've found that kind of, sort of, backs me up.)
| While Android and iOS offer high quality system fonts, one of the most common | ||
| requests from designers is to use custom fonts. For example, you may have a | ||
| custom-built font from a designer, or maybe you downloaded a font from | ||
| While Android and iOS offer high quality system fonts, |
There was a problem hiding this comment.
While -> Although
(to avoid implying time, which might confuse non-native english speakers)
| returns that. If not, it returns the App theme. | ||
| The `Theme.of(context)` method looks up the widget tree and returns | ||
| the nearest `Theme` in the tree. If you have a stand-alone | ||
| `Theme` defined above your widget, it returns that. |
There was a problem hiding this comment.
unclear antecedent to "it". Maybe, "that's returned"?
| The `Theme.of(context)` method looks up the widget tree and returns | ||
| the nearest `Theme` in the tree. If you have a stand-alone | ||
| `Theme` defined above your widget, it returns that. | ||
| If not, it returns the app's theme. |
There was a problem hiding this comment.
ditto. "the app's theme is returned"?
| the user navigates to the search screen, we can focus the search term text field. | ||
| flow. For example, say you have a search screen with a text field. | ||
| When the user navigates to the search screen, | ||
| you can focus on the search term text field. |
There was a problem hiding this comment.
"focus on" isn't right here. Maybe "set the focus to the text field for the search term."
|
|
||
| In this recipe, we'll learn how to focus a text field as soon as it's visible | ||
| as well as how to focus a text field when a button is tapped. | ||
| In this recipe, learn how to focus on a text field as soon as it's visible, |
There was a problem hiding this comment.
"focus on" sounds wrong, throughout. "give the focus to " might be right.
https://webaim.org/techniques/keyboard/ uses "has keyboard focus", "focused element", and "receive keyboard focus", "lose focus", ... https://en.wikipedia.org/wiki/Focus_(computing) has "has the focus", "moving the focus", "getting the focus", "giving [an element]...the focus", "can receive focus", ... https://documentation.basis.com/BASISHelp/WebHelp/events/keyboard_focus_navigation_and_default_button.htm is the top results and might also be helpful.
| For more information on handling input and creating text fields, please see the | ||
| [Forms section of the cookbook](/docs/cookbook#forms). | ||
| For more information on handling input and creating text fields, | ||
| see the [Forms](/docs/cookbook#forms) section of the cookbook. |
There was a problem hiding this comment.
Other places you've used Cookbook (capitalized). (I rather like it uncapitalized.)
kwalrath
left a comment
kwalrath
left a comment
There was a problem hiding this comment.
I'm quittin' for the night, but here are comments through src/docs/cookbook/navigation/passing-data.md.
| steps. | ||
| To retrieve the text a user has entered into a text field, create a | ||
| [`TextEditingController`]({{site.api}}/flutter/widgets/TextEditingController-class.html) | ||
| and supply it to a `TextField` in the next steps. |
There was a problem hiding this comment.
-> and supply it to a TextField or TextFormField.
| [`TextEditingController`]({{site.api}}/flutter/widgets/TextEditingController-class.html) | ||
| and supply it to a `TextField` in the next steps. | ||
|
|
||
| Once a `TextEditingController` is supplied to a `TextField` or `TextFormField`, |
| are finished using it. This will ensure we discard any resources used by the | ||
| object. | ||
| {{site.alert.note}} | ||
| It is important to `dispose` of the `TextEditingController` when |
There was a problem hiding this comment.
It is important to dispose -> Important: Call dispose on the ...
when -> when you've
| to a `TextField` or `TextFormField` Widget as the `controller` property. | ||
| Now that you have a `TextEditingController`, wire it up | ||
| to a specific text field. To do this, supply the `TextEditingController` | ||
| to a `TextField` or `TextFormField` widget as the `controller` property. |
There was a problem hiding this comment.
Could reduce redundancy and shorten this by saying something like this:
wire it up to a text field, using the controller property:
|
|
||
| In this example, we will display an alert dialog with the current value of | ||
| the text field when the user taps on a floating action button. | ||
| In this example, display an alert dialog with the current value of |
There was a problem hiding this comment.
This reads a little funny. I'm tempted to switch to "The following code displays...".
| the item that's been tapped. | ||
|
|
||
| Remember: Screens are Just Widgets™. | ||
| In this example, create a List of Todos. |
There was a problem hiding this comment.
List -> list
(to be consistent with the rest of the page)
Maybe also Todo -> todo unless it's in code font? Or consistently use Todo (not todo).
| For example, you might want to pass information about | ||
| the item that's been tapped. | ||
|
|
||
| Remember: Screens are Just Widgets™. |
There was a problem hiding this comment.
I don't know if we should be using trademarks this lightly.
|
|
||
| Since it's a normal `StatelessWidget`, we'll simply require that users creating | ||
| the Screen pass through a `Todo`! Then, we'll build a UI using the given Todo. | ||
| Since it's a normal `StatelessWidget`, require that the user |
There was a problem hiding this comment.
Unclear what "it" refers to.
| Since it's a normal `StatelessWidget`, we'll simply require that users creating | ||
| the Screen pass through a `Todo`! Then, we'll build a UI using the given Todo. | ||
| Since it's a normal `StatelessWidget`, require that the user | ||
| enters a `Todo` in the UI. Then, build the UI using the given Todo. |
There was a problem hiding this comment.
that the user enters -> the user to enter
| [`onTap`]({{site.api}}/flutter/material/ListTile/onTap.html) | ||
| callback for our `ListTile` Widget. Within our `onTap` callback, we'll once | ||
| again employ the [`Navigator.push`]({{site.api}}/flutter/widgets/Navigator/push.html) | ||
| To achieve this, write an |
There was a problem hiding this comment.
Unclear what "this" refers to.
kwalrath
left a comment
kwalrath
left a comment
There was a problem hiding this comment.
My brain is toast. Here are my review comments (pretty minor, iirc) through docs/cookbook/testing/integration/scrolling.md.
| ## 2. Add a button that launches the selection screen | ||
|
|
||
| Now, we'll create our SelectionButton. Our selection button will: | ||
| Now, create the SelectionButton. The selection button: |
There was a problem hiding this comment.
button -> button does the following
(But how can a button wait?!)
| Now, we'll need to build a selection screen! It will contain two buttons. When | ||
| a user taps on a button, it should close the selection screen and let the home | ||
| screen know which button was tapped! | ||
| Now, build a selection screen. It contains two buttons. When |
| screen know which button was tapped. | ||
|
|
||
| For now, we'll define the UI, and figure out how to return data in the next | ||
| For now, define the UI, and figure out how to return data in the next |
There was a problem hiding this comment.
For now... next step? Maybe:
This step defines the UI. The next step adds code to return data.
(or "will add"... that seems justified)
| Now, we'll want to update the `onPressed` callback for both of our buttons! In | ||
| order to return data to the first screen, we'll need to use the | ||
| [`Navigator.pop`]({{site.api}}/flutter/widgets/Navigator/pop.html) | ||
| Now, update the `onPressed()` callback for both of our buttons. In |
| order to return data to the first screen, we'll need to use the | ||
| [`Navigator.pop`]({{site.api}}/flutter/widgets/Navigator/pop.html) | ||
| Now, update the `onPressed()` callback for both of our buttons. In | ||
| order to return data to the first screen, use the |
|
|
||
| // Call the `main()` function of your app or call `runApp` with any widget you | ||
| // are interested in testing. | ||
| // Call the `main()` function of the app or call `runApp` with |
| method scrolls through a list until a specific widget is visible. | ||
|
|
||
| While all three methods work for specific use-cases, | ||
| `scrollUntilVisible` is oftentimes the most robust option. Why? |
| While all three methods work for specific use-cases, | ||
| `scrollUntilVisible` is oftentimes the most robust option. Why? | ||
|
|
||
| 1. If using the `scroll()` method alone, you might incorrectly assume |
There was a problem hiding this comment.
Should be bulleted, not numbered list.
| renders items on-demand, whether a particular widget has been | ||
| rendered can depend on the size of the screen. | ||
|
|
||
| Therefore, rather than assuming that you know the height of all the items |
There was a problem hiding this comment.
Therefore................................... 👎
|
|
||
| ```dart | ||
| // Imports the Flutter Driver API | ||
| // Imports the Flutter Driver API. |
There was a problem hiding this comment.
Flutter Driver? Flutter driver? flutter_driver?
kwalrath
left a comment
kwalrath
left a comment
There was a problem hiding this comment.
I did a very quick once-over of the remaining changes and didn't notice anything wrong. (very quick :)
4 participants
Inspired by PR #2048, which I'm closing.
Also fixes: #2630, which I'm also closing.
Staged: https://sz-flutter.firebaseapp.com/docs/development/packages-and-plugins/using-packages