[103] First attempt at notched corner text fields and all other 103 features #8

willlarche · 2018-04-18T02:18:11Z

Ready for final review.

HansMuller · 2018-04-18T20:43:25Z

MDC-101/complete/lib/app.dart

@@ -6,7 +6,7 @@ import 'login.dart';
 class ShrineApp extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
-    return new MaterialApp(
+    return MaterialApp(


If you're using explicit new/const then ...

HansMuller · 2018-04-18T20:44:34Z

MDC-101/complete/lib/login.dart

-class LoginPage extends StatelessWidget {
+class LoginPage extends StatefulWidget {
+  @override
+  LoginPageState createState() {


This is boilerplate and the conventional boilerplate is:

_LoginPageState createState => new _LoginPageState();

HansMuller · 2018-04-18T20:44:53Z

MDC-101/complete/lib/login.dart

+  }
+}
+
+class LoginPageState extends State<LoginPage> {


_LoginPageState

HansMuller · 2018-04-18T20:46:11Z

MDC-102/complete/lib/login.dart

@@ -1,6 +1,16 @@
 import 'package:flutter/material.dart';

-class LoginPage extends StatelessWidget {
+class LoginPage extends StatefulWidget {


See earlier comments about this same class

HansMuller · 2018-04-18T20:46:54Z

MDC-103/complete/lib/app.dart

+class ShrineApp extends StatelessWidget {
+  ThemeData _customTheme(BuildContext context) {
+    return Theme.of(context).copyWith(
+          accentColor: kShrineBrown,


2 space indent throughout this file

Formatter misbehaving. Using local variable for theme made it happy. Done.

HansMuller · 2018-04-18T20:47:58Z

MDC-103/complete/lib/app.dart

+          primaryIconTheme:
+              _customIconTheme(Theme.of(context).primaryIconTheme),
+          inputDecorationTheme:
+              InputDecorationTheme(border: NotchedCornerBorder()),


Had to be const. I'm guessing because it's a const constructor.

HansMuller · 2018-04-18T20:48:51Z

MDC-103/complete/lib/app.dart

+          buttonTheme: const ButtonThemeData(
+            textTheme: ButtonTextTheme.accent,
+          ),
+          textTheme: _customTextTheme(Theme.of(context).textTheme),


Just do this at the outset:

final ThemeData theme = Theme.of(context);

HansMuller · 2018-04-18T20:59:20Z

MDC-103/complete/lib/app.dart

+    return original.copyWith(color: kShrineBrown);
+  }
+
+  TextTheme _customTextTheme(TextTheme original) {


This is a reasonable approach to take however I think we'd prefer to start with a const ThemeData value. Will follow up with proposal for how to do that.

HansMuller · 2018-04-18T21:04:15Z

MDC-103/complete/lib/app.dart

+    return original.copyWith(color: kShrineBrown900);
+  }
+
+  TextTheme _customTextTheme(TextTheme original) {


This is a reasonable way to handle creating such a TextTheme but I think we'd prefer to define the entire Theme with a ThemeData value. Will follow up about how to do that

HansMuller · 2018-04-18T21:05:25Z

MDC-103/complete/lib/home.dart

+          ),
+        ],
+      ),
+      body: new Center(


Is this Center widget really necessary?

Nope! Removed.

HansMuller · 2018-04-19T23:20:29Z

MDC-103/complete/lib/app.dart

+import 'supplemental/theming.dart';
+
+class ShrineApp extends StatelessWidget {
+  ThemeData _customTheme(BuildContext context) {


Shrine's custom theme can be defined in terms of a ThemeData value; it shouldn't be necessary to compute anything vis Theme.of(). Here's how I'd suggest coding it.

TextTheme _buildShrineTextTheme(TextTheme base) { return base.copyWith( headline: base.headline.copyWith( fontWeight: FontWeight.w500, ), title: base.title.copyWith( fontSize: 18.0 ), caption: base.caption.copyWith( fontWeight: FontWeight.w400, fontSize: 14.0, ), ).apply( fontFamily: 'Rubik', displayColor: kShrineBrown900, bodyColor: kShrineBrown900, ); } ThemeData _buildShrineTheme() { final ThemeData base = new ThemeData.light(); return base.copyWith( accentColor: kShrineBrown900, primaryColor: kShrinePink100, buttonColor: kShrinePink100, scaffoldBackgroundColor: kShrineBackgroundWhite, cardColor: kShrineBackgroundWhite, textSelectionColor: kShrinePink100, errorColor: kShrineErrorRed, buttonTheme: const ButtonThemeData( textTheme: ButtonTextTheme.accent, ), primaryIconTheme: base.iconTheme.copyWith( color: kShrineBrown900 ), inputDecorationTheme: new InputDecorationTheme( border: new NotchedCornerBorder() ), textTheme: _buildShrineTextTheme(base.textTheme), primaryTextTheme: _buildShrineTextTheme(base.primaryTextTheme), accentTextTheme: _buildShrineTextTheme(base.accentTextTheme), ); } final ThemeData _shrineTheme = _buildShrineTheme(); ... new MaterialApp( theme: _shrineTheme, ... )

HansMuller

LGTM with some minor suggestions

HansMuller · 2018-04-23T15:31:58Z

MDC-103/complete/lib/app.dart

+      home: new HomePage(),
+      initialRoute: '/login',
+      onGenerateRoute: _getRoute,
+      theme: _buildShrineTheme(),


There's no need to build the Shrine theme each time and doing so clouds the fact that its a constant value. Create a final global and use that instead:

final ThemeData _kShrineTheme = _buildShrineTheme();

To do this, I have to move the variable and all the functions involved to be outside the class. That's ok, right? Global for all of them?

Yes. Global and file private.

HansMuller · 2018-04-23T15:34:09Z

MDC-103/complete/lib/home.dart

+
+import 'supplemental/asymmetric_grid.dart';
+
+class HomePage extends StatefulWidget {


There doesn't appear to be any reason to make this widget stateful.

HansMuller · 2018-04-23T15:37:41Z

MDC-103/complete/lib/login.dart

+  @override
+  Widget build(BuildContext context) {
+    return new Scaffold(
+      backgroundColor: kShrineSurfaceWhite,


Already defined by the Shrine theme.

MDC-103/complete/lib/login.dart

HansMuller · 2018-04-23T15:40:15Z

MDC-103/complete/lib/login.dart

+    return new Theme(
+      child: child,
+      data: Theme.of(context).copyWith(
+            primaryColor: color,


fix the indent

Done. Removed the trailing comma and it kept it all on one line.

HansMuller · 2018-04-23T15:53:43Z

MDC-103/complete/lib/supplemental/asymmetric_grid.dart

+import '../model/product.dart';
+import '../product_columns.dart';
+
+List<Container> buildGridCardsAsymmetric(BuildContext context) {


This should be a widget that returns a ListView and has a List<Product> parameter.

HansMuller · 2018-04-23T15:55:26Z

MDC-103/complete/lib/home.dart

+          ),
+        ],
+      ),
+      body: new ListView(


Should be:

new ProductsView(products: getAllProducts())

HansMuller · 2018-04-23T15:56:10Z

MDC-103/complete/pubspec.yaml

@@ -0,0 +1,29 @@
+name: mdc_101_complete
+description: >


Does the > belong here?

I think I assumed the character limit applied to the yaml as well. Done.

HansMuller · 2018-04-23T15:57:28Z

MDC-103/complete/lib/supplemental/asymmetric_grid.dart

+}
+
+int oddCasesIndex(int input) {
+  return (input / 2).ceil() * 3 - 1;


I assume you want to assert(input > 0) here?

HansMuller · 2018-04-23T15:59:23Z

MDC-103/complete/lib/supplemental/asymmetric_grid.dart

+    return const <Container>[];
+  }
+
+  return List.generate(listItemCount(products.length), (int index) {


This would be a good place to include a comment that explains what you're up to here.

willlarche requested review from mjohnsullivan, filiph, redbrogdon, HansMuller and maryx April 18, 2018 02:18

HansMuller reviewed Apr 18, 2018

View reviewed changes

willlarche added 24 commits April 18, 2018 18:46

[102] Starting data model.

1fadce5

[102] Dummy data.

a772890

[102] Fake data.

af4cab3

[102] Card currency formatting.

adef0e7

[102] Starting to make the currency localized.

7ee8838

[102] Sync.

2a45e8f

[102] Import order correction.

ec6cefe

[103] Initial commit.

d11cb4d

[103] Sync.

766e330

[103] Theming start.

81153d5

[103] Lots of styling done.

1da2372

[103] Sync.

a749a22

[102] rebase fix and Dart 2.

bad42d9

[103] font files.

64ffef9

[103] Changes from 102 PR.

fe8e68b

[103] Formatting.

bbf5536

[103] No rounded corners.

dc4b5f5

[103] Beginning horizontal scroll.

2a2760b

[103] Compiling again in Dart 2.

a02c671

[101] Adding back in consts and news.

f1dc58a

[103] Sync.

99bbede

[103] Consts and news.

b4cf2c6

[103] New and Const.

d09d2a0

[103] Removing unnecessary function.

cde618f

willlarche added 10 commits April 18, 2018 18:46

[103] Formatting.

18fdc36

[103] New const correction.

c99f350

[103] Removing unused widget.

38d6c0d

[101] Missing explicit new.

71a46ca

[103] Explicit news.

e92ffe7

[103] Explicit news and consts.

b69ca4b

[103] Explicit const.

786516d

[103] Notched corners except animation.

b9ea25e

[103] Colors.

3840981

[103] Notched corner animation.

7d17497

HansMuller reviewed Apr 19, 2018

View reviewed changes

willlarche added 5 commits April 20, 2018 18:31

[103] Custom widget for the primary color setting on the text fields.

ccddf26

[103] Updated Theme code from Hans.

7bf5092

[103] More work on shapes.

1b4c4d5

[103] Correction to text fields.

f362843

[103] Shape added to flat button for ink.

922eda3

HansMuller approved these changes Apr 23, 2018

View reviewed changes

willlarche added 9 commits April 23, 2018 12:32

[103] PR feedback.

cae18fe

[103] PR feedback.

87f030e

[103] Renaming.

9285a58

[103] Formatting.

59c8de1

[103] PR feedback.

a15c3a3

[103] PR feedback.

44cee9f

[103] Comments.

6d8ccb1

[103] PR feedback about lerp.

5d1f717

[103] Lerping correctiong.

faf5834

willlarche merged commit f458311 into material-components:master Apr 23, 2018

willlarche deleted the feature-103-complete branch April 23, 2018 17:52

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 'supplemental/asymmetric_grid.dart';

		class HomePage extends StatefulWidget {

[103] First attempt at notched corner text fields and all other 103 features #8

[103] First attempt at notched corner text fields and all other 103 features #8

Uh oh!

Conversation

willlarche commented Apr 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

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

willlarche commented Apr 18, 2018 •

edited

Loading