[102] Complete #3

willlarche · 2018-04-05T00:50:19Z

Dependent on #2

Ignore anything in a directory with 101. It's awaiting the earlier pull request to set those files straight.

Also, all the data an images is temporary. New images coming next week.

redbrogdon · 2018-04-05T21:13:05Z

MDC-102/complete/lib/home.dart

+              Expanded(
+                  child: Padding(
+                padding: EdgeInsets.fromLTRB(
+                  16.0,


Did the flutter formatter put all these numbers on their own lines? Because that's the first time I've seen positional parameters split out like that.

I think it does that only if the last one ends in a comma.

Mind blown.

redbrogdon · 2018-04-05T21:14:28Z

MDC-102/complete/lib/home.dart

+                    ),
+                    SizedBox(height: 8.0),
+                    // TODO(larche): Make subtitle2 when available
+                    Text('${products[index].priceString}',


products[index].priceString is already a string, unless I'm mistaken. The string interpolation should be unnecessary here.

Oh, duh. Yeah :)

redbrogdon · 2018-04-05T21:28:19Z

MDC-102/complete/lib/model/product.dart

+
+  final String priceString;
+
+  const Product(category, id, isFeatured, name, price, priceString)


Consider named parameters here with @required from dart/meta. Six positional parameters is a lot to remember/get right each time.

Having this constructor isn't actually necessary anymore. It was when I was trying to have the formatter live in the class and format on creation.

But did the @required on the next one.

redbrogdon · 2018-04-05T21:44:41Z

MDC-102/complete/lib/model/data.dart

+
+import 'product.dart';
+
+List<Product> allProducts = generateProducts();


I'm not sure how much kibitzing you're looking for here, but I would recommend refactoring this code a bit

Get rid of allProducts, which I don't think is being used.

Change formatter to be a parameter in the call to generateProducts().

Change getData to return Future<List<Product>> rather than Future<Null>

Change _HomePageState to have a member variable called allProducts of type List<Product>

In the initState method for HomePage (line 61) of previous file, do this:

getData().then((list)` => setState(() { allProducts = list; }); );

That puts a copy of the products to be displayed into the State object, and avoids having an empty setState call, which I've heard is a code smell for Flutter.

So, I couldn't do exactly that because I needed to have a default value for the formatter incase the async method took too long (which I've seen.)

maryx · 2018-04-06T01:09:21Z

MDC-102/complete/lib/home.dart

+            icon: Icon(Icons.menu),
+            onPressed: () {
+              print('Menu button');
+            }),


General comment, I think it might be a style guidance to put a comma after each parameter, so the end ) of IconButton matches the indent of leading. Same for below, where there'd be a comma after childAspectRatio: 8.0 / 9.0, so that the last ), matches the child indent. Might want to verify w Flutter team

Yes you are correct! I had that wrong.

I went thru and added them anywhere I could catch. It really improved the formatting!

maryx · 2018-04-06T01:10:12Z

MDC-102/complete/lib/main.dart

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


I think this can just be ShrineApp(), no need new

redbrogdon · 2018-04-06T21:19:13Z

MDC-102/complete/lib/home.dart

+}
+
+class _HomePageState extends State<HomePage> {
+  List<Product> allProducts = generateProducts(null);


Rather than catching and replacing this null at line 15 of data.dart, I'd recommend just passing NumberFormat.simpleCurrency() here. If the home page is responsible for getting the locale and providing the formatter in initState, it seems reasonable to me that it would also be responsible for choosing the fallback formatter and providing that.

redbrogdon

Two mostly opinionated comments.

redbrogdon · 2018-04-09T16:05:21Z

MDC-102/complete/lib/home.dart

+                    16.0,
+                    12.0,
+                    16.0,
+                    8.0,


Nit: if the trailing comma after "8.0" is what's making these appear on their own lines (and unless you have other guidance from Hans or whomever), I'd remove it. It's just bonkers to me to have these spread out so much.

Well, I had it taken out and then I asked around the office and people said yes to trailing commas. They didn't see this case tho. Maybe this is an exception. @HansMuller , what do you prefer?

I agree, just one line is much easier to read. In cases like this we usually take a break from the trailing comma dogma.

EdgeInsets.fromLTRB(16.0, 12.0, 16.0, 8.0),

redbrogdon · 2018-04-09T16:14:36Z

MDC-102/complete/lib/home.dart

+    super.initState();
+
+    getLocale().then((locale) => setState(() {
+          formatter = NumberFormat.simpleCurrency(


I don't have anything real to back this up, but for some reason I feel like the function passed to setState should be as small as possible, and just do assignments to the state object. I'm not sure if that's an official style point or just something I've picked up from various examples, but to me this code:

getLocale().then((locale) { formatter = NumberFormat.simpleCurrency( locale: locale, decimalDigits: 0, ); setState(() { allProducts = generateProducts(formatter); }); });

Seems cleaner, since all it's doing in setState is setting things on the State object. Hans or Filip can correct me if I'm being weird or the closures don't work this way, of course. 😃

Check out the changes Hans asked for. This is actually out. He knew a better way to get the locale.

HansMuller

The gist of much of the feedback is: do the localization late, when the UI is built (not in initState() or class vars).

HansMuller · 2018-04-09T17:56:08Z

MDC-102/complete/lib/home.dart

+                      // TODO(larche): Make headline6 when available
+                      Text(
+                        products[index].name,
+                        style: Theme.of(context).textTheme.title,


Theme.of() is somewhat expensive, so best to cache a ThemeData value at the top of the build function. This is really common, conventionally the variable is called theme:

final ThemeData theme = Theme.of(context);

HansMuller · 2018-04-09T17:58:50Z

MDC-102/complete/lib/home.dart

+    return [];
+  }
+
+  List<Card> cards = List.generate(


There doesn't seem to be any need to create this variable. Note also: you could make the formatting a little more conventional like this:

return List.generate(products.length, (int index) { return Card( child: ... ); }).toList();

Done! But why do I need the toList()? Doesn't List.generate return a List?

Yes, you are correct, the toList() isn't needed.

HansMuller · 2018-04-09T17:59:18Z

MDC-102/complete/lib/home.dart

+
+  List<Card> cards = List.generate(
+    products.length,
+    (int index) => Card(


We only use => for one-liners that return a value

HansMuller · 2018-04-09T18:14:59Z

MDC-102/complete/lib/home.dart

+  void initState() {
+    super.initState();
+
+    getLocale().then((locale) => setState(() {


You shouldn't call setState() from initState(). I don't think you want to do this here at all. You don't want to call getLocale() either, since Flutter's idea of Locale depends on where a widget is in its tree.

Use Localizations.localeOf(context) to look up the locale when you need it (you'll have to convert it to a String to use it with dart:intl, like locale.toString())

HansMuller · 2018-04-09T18:21:16Z

MDC-102/complete/lib/home.dart

+
+List<Card> _buildGridCards(BuildContext context, List<Product> products) {
+  if (products == null || products.isEmpty) {
+    return [];


return const <Card>[]

HansMuller · 2018-04-09T18:30:46Z

MDC-102/complete/lib/model/data.dart

+  return localeString;
+}
+
+List<Product> generateProducts(NumberFormat formatter) {


You could just export a constant list here, like kAllProducts. If you're really trying to model an implicit lookup, then getAllProducts().

Done. I went with getAllProducts() since that will set us up for category filtering later.

HansMuller · 2018-04-09T18:31:15Z

MDC-102/complete/lib/model/data.dart

+
+List<Product> generateProducts(NumberFormat formatter) {
+  return <Product>[
+    Product(


To be consistent: 2 space indents.

I think it is 2 space indents. But the kerning is weird and you can't easily tell.

Check the whole file...

HansMuller · 2018-04-09T18:31:48Z

MDC-102/complete/lib/model/product.dart

+    @required this.isFeatured,
+    @required this.name,
+    @required this.price,
+    @required this.priceString,


The localized price string should be computed by the widget that displays Product, not here.

HansMuller · 2018-04-09T18:32:21Z

MDC-102/complete/lib/model/product.dart

+  /// Formatted currency representation of price. Localized if possible.
+  final String priceString;
+
+  const Product({


Put the constructor first; assert that the parameters aren't null

Like this:

class Product { const Product({ @required this.category, @required this.id, @required this.isFeatured, @required this.name, @required this.price, }) : assert(category != null), assert(id != null), assert(isFeatured != null), assert(name != null), assert(price != null); final Category category; final int id; final bool isFeatured; final String name; final int price; @override String toString() => "$name (id=$id)"; }

?

HansMuller · 2018-04-09T18:34:39Z

MDC-102/complete/lib/home.dart

+}
+
+class _HomePageState extends State<HomePage> {
+  List<Product> allProducts = generateProducts(NumberFormat.simpleCurrency(


This value should be computed when it's needed.

I put it inside _buildGridCards as a local variable. Lemme know if that makes sense.

HansMuller · 2018-04-10T00:40:31Z

MDC-102/complete/lib/home.dart

+    final ThemeData theme = Theme.of(context);
+    final NumberFormat formatter = NumberFormat.simpleCurrency(
+        locale: Localizations.localeOf(context).toString());
+    return List.generate(products.length, (int index) {


If you're just looking up the products by index, then using map would be simpler:

return getAllProducts().map((Product product) { return Card( ... product.name ... product.id ... etc ) }).toList();

HansMuller · 2018-04-10T22:03:38Z

LGTM

willlarche requested review from filiph, redbrogdon, HansMuller and maryx April 5, 2018 03:32

redbrogdon reviewed Apr 5, 2018

View reviewed changes

maryx reviewed Apr 6, 2018

View reviewed changes

redbrogdon reviewed Apr 6, 2018

View reviewed changes

willlarche added 23 commits April 6, 2018 22:07

[102] Buttons for AppBar.

f6ae733

[102] Copy correction.

8e7b4f1

[102] Almost complete layout.

8dd59af

[102] Starting data model.

fc52587

[102] Formatting.

2251d54

[102] Dummy data.

12f0fcd

[102] Fake data.

86a116c

[102] Formatting

dea9829

[102] Card currency formatting.

e144815

[102] Starting to make the currency localized.

8951bf4

[102] Pubspec for intl.

c5ccaaf

[102] Data work.

7976303

[102] Home layout

08ae71b

[102] Formatting

23adcf4

[102] Data.

2618336

[102] Formatting.

98e6ca6

[102] Sync.

c2b4dba

[102] Formatting.

dacac60

[102] PR feedback.

fff531c

[102] Minor refactor for data.

f5b3f9a

[102] Formatting.

3e1a1b4

[102] PR Feedback (trailing commas)

1347451

[102] Improving formatting and simplifying data model.

5d117c7

willlarche added 3 commits April 6, 2018 22:07

[102] Comment.

fea05d5

[102] Sync.

af8ea45

[102] Import order correction.

c19446d

redbrogdon reviewed Apr 9, 2018

View reviewed changes

HansMuller reviewed Apr 9, 2018

View reviewed changes

willlarche added 6 commits April 9, 2018 16:41

[102] PR feedback.

8e74c3b

[102] PR feedback.

1932f9e

[102] PR feedback around localization.

f4a9272

[102] PR feedback.

8047261

[102] PR feedback.

daf393d

[102] PR feedback.

1b35cb7

HansMuller reviewed Apr 10, 2018

View reviewed changes

willlarche added 2 commits April 10, 2018 14:52

[102] Better support for smaller screens.

f87f765

[102] Simplifying creation of cards.

f6abd24

willlarche merged commit feb63f1 into material-components:master Apr 10, 2018

willlarche deleted the feature-102-complete branch April 10, 2018 22:04

Zheng-Shuo mentioned this pull request Jun 11, 2018

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

Closed

willlarche mentioned this pull request Mar 26, 2019

description unclear on which code goes where #152

Closed

likhit-vivek mentioned this pull request May 11, 2019

Compile error when trying to run 101-starter #162

Closed


		final String priceString;

		const Product(category, id, isFeatured, name, price, priceString)


		import 'product.dart';

		List<Product> allProducts = generateProducts();


		import 'package:flutter/material.dart';

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

+.0,
+.0,
+.0,
+.0,

[102] Complete #3

[102] Complete #3

Uh oh!

Conversation

willlarche commented Apr 5, 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!

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

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

willlarche commented Apr 5, 2018 •

edited

Loading