[101] Complete #2

willlarche · 2018-03-31T02:00:31Z

Magic numbers and strings are for educational simplicity.

maryx · 2018-04-01T00:33:20Z

MDC-101/complete/lib/login.dart

+            decoration: InputDecoration(
+              border: const UnderlineInputBorder(),
+              filled: true,
+              labelText: "Username",


Nit, do we care if we use single vs. double quotes in the same file? You use them both in this file.

maryx · 2018-04-01T00:33:59Z

MDC-101/complete/lib/login.dart

+  @override
+  Widget build(BuildContext context) {
+    return Scaffold(
+        body: SafeArea(


Nit, formatting. If you're using IntelliJ there should be a Tools -> Autoformat with dartfmt you can use, and map that to a keyboard shortcut.

I ran it! That's the way it did it.

Looks like it may have a bug? Don't know. Fixed it tho.

maryx · 2018-04-01T00:35:20Z

MDC-101/complete/lib/login.dart

+              ],
+            ),
+          ),
+          SizedBox(height: 121.0),


I thought Material was about having heights/font sizes etc. in multiples of 4? E.g this would be 120.0 and the SizedBox below would be 12.0 or 16.0? This might be a team-specific guideline so feel free to ignore.

Good point. My mocks were 121 but no need to bring attention to it.

maryx · 2018-04-01T00:36:50Z

MDC-101/complete/lib/main.dart

+
+import 'package:flutter/material.dart';
+
+void main() => runApp(new ShrineApp());


I think you can remove the new? (probably not on the tip of tree of Flutter due to flutter/flutter#15590 but theoretically, you should be?)

HansMuller

Looks like a good start

HansMuller · 2018-04-02T16:05:23Z

MDC-101/complete/lib/login.dart

+        padding: EdgeInsets.symmetric(horizontal: 24.0),
+        children: <Widget>[
+          SizedBox(height: 80.0),
+          Container(


This container isn't needed.

HansMuller · 2018-04-02T16:06:54Z

MDC-101/complete/lib/login.dart

+          SizedBox(height: 120.0),
+          TextField(
+            decoration: InputDecoration(
+              border: const UnderlineInputBorder(),


This is the default, you don't need to specify it.

HansMuller · 2018-04-02T16:09:25Z

MDC-101/complete/lib/login.dart

+                  child: Text('NEXT'),
+                  onPressed: () {
+                    Navigator.pop(context);
+                  })


Add a trailing comma and then put the closing paren under the class name (same as everywhere else):

RaisedButton( child: Text('NEXT'), onPressed: () { Navigator.pop(context); }, ),

HansMuller · 2018-04-02T16:11:37Z

MDC-101/complete/lib/login.dart

+  @override
+  Widget build(BuildContext context) {
+    return Scaffold(
+        body: SafeArea(


There's an extra indent here

That indent is coming from dartfmt. Think it could be somehow on purpose?

I do not know, however it's inconsistent with style in the Flutter codebase.

Looks like my dartfmt was out of date. updating and rerunning.

HansMuller · 2018-04-02T16:12:29Z

MDC-101/complete/lib/login.dart

+          )
+        ],
+      ),
+    ));


It looks like things didn't fit, so you ended up with a '))' line, because there's an extra indent at the top?

redbrogdon · 2018-04-02T17:35:40Z

MDC-101/complete/lib/login.dart

+      child: ListView(
+        padding: EdgeInsets.symmetric(horizontal: 24.0),
+        children: <Widget>[
+          SizedBox(height: 80.0),


Are these SizedBox instances being used to block out padding for the layout? If so, is that considered the correct way to do it, rather than wrapping the following Widget in Padding(padding: EdgeInsets.only(top:80.0))?

That's a good point. I find that this style of inserting space between list items, where the gaps are defined explicitly with sized boxes, is easier to read than when the padding is part of the items themselves.

I can do either! I don't know what would be more expected. From an iOS POV, I wouldn't expect any of this to work without a bunch more layout code so my instincts don't apply.

redbrogdon · 2018-04-04T20:29:08Z

MDC-101/complete/lib/app.dart

+      title: 'Shrine',
+      home: HomePage(),
+      initialRoute: '/login',
+      onGenerateRoute: _getRoute,


Another comment that's more a question:

Is there a definite benefit to using onGenerateRoute here rather than supplying a map via the routes property? It may just be the examples I happen to be looking at right now, but that seems like a popular option:

https://github.com/brianegan/flutter_architecture_samples/blob/master/example/scoped_model/lib/app.dart

I don't know that you can set the fullScreenDialog option that way, though, which may be a dealbreaker.

+1, I was not even aware of onGenerateRoute until now.

It's all new to me! Need me to change it?

Oh, actually, I used to have the routes map. But I couldn't get the pop to be a modal-style dismiss.

Discussed offline, and this is definitely the better way to go. I'm now curious what other functionality you lose (if any) by using a straight up route table.

redbrogdon · 2018-04-04T20:30:33Z

MDC-101/complete/lib/login.dart

+              ),
+            ),
+            SizedBox(height: 32.0),
+            Row(


FWIW, I'm using a ButtonBar here in my material example for the DevByte.

Think I should? @HansMuller

Done! I love it.

filiph

LGTM with some nits

filiph · 2018-04-04T23:08:14Z

MDC-101/complete/lib/app.dart

+  }
+
+  Route<dynamic> _getRoute(RouteSettings settings) {
+    if (settings.name == '/login') {


[nit] Consider using the guard pattern here: if you can have a short (ideally one-line) if-else statement, use that instead of a longer if-else statement.

if (settings.name != '/login') return null; return MaterialPageRoute<void>( ... );

Of course, this depends. If you expect more routes, then the guard pattern would be silly.

filiph · 2018-04-04T23:10:45Z

MDC-101/complete/lib/app.dart

+      title: 'Shrine',
+      home: HomePage(),
+      initialRoute: '/login',
+      onGenerateRoute: _getRoute,


+1, I was not even aware of onGenerateRoute until now.

filiph · 2018-04-04T23:28:47Z

MDC-101/complete/lib/home.dart

@@ -0,0 +1,20 @@
+import 'package:flutter/material.dart';
+
+class HomePage extends StatefulWidget {


You don't need stateful here, but I assume you know this.

There's a cool IDE feature in both IntelliJ and VS Code that lets you convert StatelessWidget to StatefulWidget with two keystrokes. You might want to teach this in the next step, because it's super helpful.

It was because later I need it. But instead I'll take that moment to teach them, yeah.

filiph · 2018-04-04T23:31:32Z

MDC-101/complete/lib/main.dart

+
+import 'app.dart';
+
+void main() => runApp(ShrineApp());


Just FYI: optional new/const might not make it for I/O. That's a worst case scenario, but after the recent snafu, it's a lot more probable than before.

filiph · 2018-04-04T23:32:07Z

MDC-101/complete/pubspec.yaml

@@ -0,0 +1,21 @@
+name: mdc_101_complete
+description: Learn the basics of using Material Components by building a simple app with core components.


You can stay inside width limit with

description: > Learn the basics of using Material Components by building a simple app with core components.

filiph · 2018-04-04T23:32:57Z

MDC-101/complete/pubspec.yaml

+    sdk: flutter
+
+
+# The following section is specific to Flutter.


Since you already got rid of all the other comments, I think you can get rid of this one as well. (And the preceding blank line.)

redbrogdon · 2018-04-06T21:03:58Z

MDC-101/complete/lib/app.dart

+      title: 'Shrine',
+      home: HomePage(),
+      initialRoute: '/login',
+      onGenerateRoute: _getRoute,


Discussed offline, and this is definitely the better way to go. I'm now curious what other functionality you lose (if any) by using a straight up route table.

willlarche added 4 commits March 30, 2018 21:48

[101] Complete.

a1ab2c7

[101] Pubspec.

244446e

[101] .gitignore.

0ac931c

[101] .metadata.

fe6b56b

willlarche requested review from wmleler, filiph, redbrogdon and maryx March 31, 2018 02:01

willlarche added 2 commits March 30, 2018 22:15

[101] Correcting initial navigation.

631ed80

[101] Made opening route modal

3f314dd

maryx reviewed Apr 1, 2018

View reviewed changes

willlarche added 2 commits April 1, 2018 15:08

[101] PR feedback.

62c6a2f

[101] Removing unused import.

6f87fe9

HansMuller reviewed Apr 2, 2018

View reviewed changes

[101] PR feedback.

d1ad4c7

redbrogdon reviewed Apr 2, 2018

View reviewed changes

willlarche added 4 commits April 2, 2018 15:05

[101] Formatting.

d37d8d6

[101] Formatting.

d5ed365

[101] Formatting.

a1fb7ff

[101] Formatting.

1bd230f

redbrogdon reviewed Apr 4, 2018

View reviewed changes

[101] Adding password obscuring.

cee0927

filiph approved these changes Apr 4, 2018

View reviewed changes

willlarche added 3 commits April 4, 2018 17:06

[101] PR feedback.

d213332

[101] Replacing sizedBox and row with buttonBar.

ca3d97d

[101] Getting rid of analyzer warning.

5ffab0c

willlarche mentioned this pull request Apr 5, 2018

[102] Complete #3

Merged

redbrogdon approved these changes Apr 6, 2018

View reviewed changes

willlarche merged commit b4ada31 into material-components:master Apr 6, 2018

willlarche deleted the feature-101-complete branch April 6, 2018 22:41

Zheng-Shuo mentioned this pull request Jun 11, 2018

MDC-103 Flutter: Material Theming with Color, Shape, Elevation, and Type (Flutter) #36

Closed

likhit-vivek mentioned this pull request May 11, 2019

Compile error when trying to run 101-starter #162

Closed


		import 'package:flutter/material.dart';

		void main() => runApp(new ShrineApp());

		@@ -0,0 +1,20 @@
		import 'package:flutter/material.dart';

		class HomePage extends StatefulWidget {

		@@ -0,0 +1,21 @@
		name: mdc_101_complete
		description: Learn the basics of using Material Components by building a simple app with core components.

[101] Complete #2

[101] Complete #2

Uh oh!

Conversation

willlarche commented Mar 31, 2018

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!

HansMuller 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!

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!

filiph left a comment

Choose a reason for hiding this comment

Uh oh!