Updating getting started codelab by Sfshaza · Pull Request #1124 · flutter/website

Sfshaza · 2018-07-30T19:14:06Z

googlebot · 2018-07-30T19:53:47Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

Sfshaza · 2018-08-02T04:17:44Z

I believe this is ready to go, once it's been reviewed.

P.S. @chalin, do you know how to get rid of the WIP status?

Sfshaza · 2018-08-02T04:22:19Z

I confirm that @chalin's changes are good.

chalin

Nice updates! Please see inlined comments, some comments are meant to address discrepancies relative to the doc style guide (https://developers.google.com/style/)

chalin · 2018-08-02T13:19:07Z

-    <img src="images/startup-namer-app.gif"
+    <img src="images/startup_namer_1_opt.gif"
         alt="Animated GIF of the app that you will be building."
         style="border: 10px solid #333; border-radius: 10px; margin-bottom: 10px" >


I'd suggest dropping the border styling here: I was necessary with the old image, but it isn't anymore since the new image already shows the phone border.

chalin · 2018-08-02T13:19:34Z

@@ -6,7 +6,7 @@ image: /get-started/codelab/images/step7-themes.png
 ---

 <figure class="right-figure" style="max-width: 260px; padding-right: 10px">


We should avoid doing custom styling like this (use of class="right-figure" should be enough), but I can fix that in a followup PR.

chalin · 2018-08-02T13:21:46Z

 {:toc}

-## What you'll build
+## What you'll build in part 1


I was puzzled for a moment since I wasn't aware of Part 2.

How about adding ", Part 1" to the title? If fact, maybe adding ", Part 1" to the title is enough (i.e., we don't need to add it here).

The introductory paragraph (for the page), should mention that there is a followup "Part 2" (and link to it).

Is this page identical to https://codelabs.developers.google.com/codelabs/first-flutter-app-pt1? Regardless, the introductory paragraph should also mention the codelabs.developers part 1 and make it clear whether it is the same or different.

They will be the same, once I've updated both.

chalin · 2018-08-02T13:22:54Z

 You’ll implement a simple mobile app that generates proposed names for a
 startup company. The user can select and unselect names,
-saving the best ones. The code generates ten names at a time.
+saving the best ones. The code lazily generates ten names at a time.


This would be more accurate:

~~The code lazily generates names, ten at a time.~~

Actually, we probably don't need to mention a number here, given the sentence that follows. So I'd simplify this sentence to:

The code lazily generates names.

chalin · 2018-08-02T13:25:12Z

 startup company. The user can select and unselect names,
-saving the best ones. The code generates ten names at a time.
+saving the best ones. The code lazily generates ten names at a time.
 As the user scrolls, new batches of names are generated.


-> As the user scrolls, more names are generated.

chalin · 2018-08-02T14:54:50Z

+    and changing the home to be a RandomWords widget.

-    Add the highlighted code below:
+    Replace the original method with the highlighted build method below:


How about replacing both (1-sentence) paragraphs by:

Update the MyApp build() method as follows:

chalin · 2018-08-02T14:56:34Z

-## Next step
-{:.no_toc}
+If you would like to extend this app, proceed to
+[part 2](https://codelabs.developers.google.com/codelabs/first-flutter-app-pt2/#0)


part 2 -> Part 2

chalin · 2018-08-02T14:58:20Z

@@ -926,14 +569,20 @@ In this codelab, you've:
 * Written Dart code.
 * Leveraged an external, third party library.


third party -> third-party

chalin · 2018-08-02T14:59:18Z

 * Used hot reload for a faster development cycle.
-* Implemented a stateful widget, adding interactivity to your app.
+* Implemented a stateful widget.
 * Created a lazily loaded, infinite scrolling list displayed with a


Let's keep this summary simple (we don't mention implementation details in the other bullets):

Created a lazily loaded, infinite scrolling list displayed ... -> Created a lazily loaded, infinite scrolling list.

chalin · 2018-08-02T15:00:42Z

+    <img src="images/startup-namer-app.gif"
+         alt="Animated GIF of the app that you will be building."
+         style="border: 10px solid #333; border-radius: 10px; margin-bottom: 10px" >
+    <center>The app at the end of step 2</center><br>


end of step 2 -> end of Part 2

Is this really meant to be "The app at the end of step 2"

chalin · 2018-08-02T15:11:05Z

do you know how to get rid of the WIP status?

No, but it shouldn't prevent us from doing a "squash and merge" later.

As for the cla: no label, I think that you can switch labels yourself.

googlebot · 2018-08-02T20:08:38Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

Sfshaza · 2018-08-02T22:30:48Z

Hi, @chalin! OK, I have incorporated most of your feedback. I realize that you will make further changes once you incorporate the auto diffs. You have my blessing.

But I'd like to get this merged this week and I'm out tomorrow. Please take a look.

chalin · 2018-08-02T22:40:40Z

Please take a look.

Gladly, but I'm not seeing the changes.

googlebot · 2018-08-02T23:44:44Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

chalin · 2018-08-03T00:27:41Z

+There is no limit to how far a user can scroll.

-The animated GIF shows how the app works at the completion of part 1:
+The animated GIF shows how the app works at the completion of Part 1.


I suppose that this should be "part 1" (lowercase) like the rest of the occurrences of the word?

chalin · 2018-08-03T00:29:18Z

do you know how to get rid of the WIP status?

Yes, remove the text "WORK IN PROGRESS" from the message of the first commit.

chalin · 2018-08-03T00:32:15Z

So it is looking good aside from two comments. (One of the comments doesn't appear on this page; it is here -- and asks if the caption for the last image is really meant to be "The app at the end of step 2"? If it is referring to step 2 of part 2, I'd just omit that caption or change it to: "The app from part 2".

Sfshaza · 2018-08-03T14:18:17Z

This PR got its knickers in a twist because Patrice and I collaborated on some of the changes. Merging.

googlebot added the cla: yes Contributor has signed the Contributor License Agreement label Jul 30, 2018

googlebot added cla: no Contributor has not signed the Contributor License Agreement and removed cla: yes Contributor has signed the Contributor License Agreement labels Jul 30, 2018

Sfshaza changed the title ~~WORK IN PROGRESS (for Patrice) Updating getting started codelab~~ Updating getting started codelab Aug 2, 2018

Sfshaza requested a review from chalin August 2, 2018 04:17

chalin suggested changes Aug 2, 2018

View reviewed changes

Sfshaza added cla: yes Contributor has signed the Contributor License Agreement and removed cla: no Contributor has not signed the Contributor License Agreement labels Aug 2, 2018

googlebot added cla: no Contributor has not signed the Contributor License Agreement and removed cla: yes Contributor has signed the Contributor License Agreement labels Aug 2, 2018

chalin reviewed Aug 3, 2018

View reviewed changes

chalin approved these changes Aug 3, 2018

View reviewed changes

sfshaza2 and others added 5 commits August 3, 2018 05:29

Draft (for Patrice) Updating getting started codelab

684d8e4

Markdown cleanup

865ed4a

Updating getting started codelab and pointing to repo code.

a3be2c6

Incorporated feedback.

d4104bf

A few final tweaks.

b5b4d9c

chalin force-pushed the codelab branch from 9fc1ebb to b5b4d9c Compare August 3, 2018 09:30

Sfshaza merged commit 0944d3a into master Aug 3, 2018

Sfshaza deleted the codelab branch August 3, 2018 14:18

		@@ -6,7 +6,7 @@ image: /get-started/codelab/images/step7-themes.png
		---

		<figure class="right-figure" style="max-width: 260px; padding-right: 10px">

		@@ -926,14 +569,20 @@ In this codelab, you've:
		* Written Dart code.
		* Leveraged an external, third party library.

Conversation

Sfshaza commented Jul 30, 2018

Uh oh!

googlebot commented Jul 30, 2018

Uh oh!

Sfshaza commented Aug 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sfshaza commented Aug 2, 2018

Uh oh!

chalin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalin commented Aug 2, 2018

Uh oh!

googlebot commented Aug 2, 2018

Uh oh!

Sfshaza commented Aug 2, 2018

Uh oh!

chalin commented Aug 2, 2018

Uh oh!

googlebot commented Aug 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalin commented Aug 3, 2018

Uh oh!

chalin commented Aug 3, 2018

Uh oh!

Sfshaza commented Aug 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Sfshaza commented Aug 2, 2018 •

edited

Loading