Skip to content

Adding shrine. #3

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
Jul 26, 2018
Merged

Adding shrine. #3

merged 16 commits into from
Jul 26, 2018

Conversation

RedBrogdon
Copy link
Contributor

No description provided.

@RedBrogdon RedBrogdon requested a review from willlarche June 27, 2018 23:39
@@ -0,0 +1,8 @@
# This file tracks properties of this Flutter project.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be wrong but I think you can drop this file too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure? It actually says in the file that it should be version-controlled.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure! Could we get more info from the Flutter team proper?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After asking around, it looks like this file stores the ID of the version of the flutter SDK used with the project so that later versions can know which changes to make when someone upgrades.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. Like a kind of lock file. In that case, it might make sense for this repo but not for the codelabs which will def have a requirement to run create.

shrine/README.md Outdated
@@ -0,0 +1,45 @@
# shrine
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shrine with capital S.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

shrine/README.md Outdated
[`scoped_model`](https://pub.dartlang.org/packages/scoped_model) to manage the
state of its shopping cart.

## Goals for this sample
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can just say Goals.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. It's not like I'm going to put my retirement goals in here or anything.

shrine/README.md Outdated
@@ -0,0 +1,45 @@
# shrine

A sample shopping app that uses Material widgets in its UI and
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is the first time you say it, could you make it Material Components widgets? I know it doesn't roll off the tongue but I'd want that phrase once in a doc like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem. Fixed.

Copy link
Member

@willlarche willlarche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you delete the Android and iOS boilerplate files since flutter create makes them?

Also, I have an icon for the app. See me in email.

shrine/README.md Outdated

## Goals

* Show how to customize Flutter's Material Compoenent widgets to produce
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spelling of Component

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch. I think that's the German spelling. :)

Fixed.

shrine/README.md Outdated

* Show how to customize Flutter's Material Compoenent widgets to produce
a unique design for an app.
* Show how to use `scoped_model` to manage an app's state and access it
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a cool video on scoped model architecture for Flutter right? Could you include a link?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we have anything specifically for scoped_model. There was an I/O talk that showed it as one of a few options, but I think that's about it.

@@ -0,0 +1,51 @@
def localProperties = new Properties()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment below.

@@ -0,0 +1,39 @@
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment below.

@@ -0,0 +1,5 @@
# Launch Screen Assets
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can have a launch screen created by the design team too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still remember that animated diamond from the mock you showed me. That was really cool.

import 'colors.dart';
import 'home.dart';
import 'login.dart';
import 'category_menu_page.dart';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alphabetize.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh. Could have sworn I optimized all the imports. Thanks for catching it.

import 'package:flutter/material.dart';
import 'package:meta/meta.dart';

import 'model/product.dart';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

// limitations under the License.

import 'package:scoped_model/scoped_model.dart';
import 'product.dart';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

import 'product.dart';
import 'products_repository.dart';

double _shippingCostPerItem = 7.0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alphabetize.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@RedBrogdon
Copy link
Contributor Author

Thanks for the review! PTAL.

About the presence of the Android and iOS subprojects, I think this is likely going to be an ongoing difference between your codelab version of Shrine and my example app version of it. The copy here is meant to run out of the box (without anyone running flutter create to make their own project), so it needs those files in place.

.pub/
.idea
.atom
.flutter-plugins
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider .vscode too. Michelle added that one in the codelabs repo.

#include "AppDelegate.h"
#include "GeneratedPluginRegistrant.h"

@implementation AppDelegate
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glad you're using the ObjC version. Will be lighter!

@RedBrogdon
Copy link
Contributor Author

@willlarche PTAL when you get the chance. I did a giant merge between this and the current state of 104-complete.

@@ -23,14 +23,19 @@ abstract class BuiltComplexObject
@nullable
double get aDouble;

@nullable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I've literally never noticed this annotation before!

@@ -7,7 +7,7 @@
part of 'built_complex_object.dart';

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who generates this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's from the built_value package. I've managed to double up some commits in this PR. This is from our JSON example app.

// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'package:flutter/material.dart';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love that you included tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll be adding some for Shrine, hopefully, in the near future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll use them too.

import 'package:flutter/material.dart';
import 'package:scoped_model/scoped_model.dart';

import 'app.dart';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we found you need to package: this import because of a sometimes bug in Dart.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only in main tho.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll double-check. The only package-or-not issue I've run into so far, though, is that if you import a file twice in your app, once with package and once without, Dart will consider them two separate namespaces and double up all the exports.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the issue I've been referencing (if you haven't seen it): dart-lang/sdk#33076.

if (_productsInCart[productId] == 1) {
_productsInCart.remove(productId);
} else {
_productsInCart[productId] -= 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might look nice to have -- to match the one above. Or have the one above be +=.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


class _ShoppingCartPageState extends State<ShoppingCartPage> {
List<Widget> _createShoppingCartRows(AppStateModel model) {
return model.productsInCart.keys
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indenting of the lines following this doesn't seem quite right but I'm not an expert enough on the style guide to know for sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flutter format did it this way, I swear. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants