Update to latest Flutter SDK #14

rxlabz · 2017-08-22T12:03:57Z

No description provided.

sethladd · 2017-08-23T15:41:53Z

example/pubspec.yaml

+  flutter_test:
+    sdk: flutter
+  flutter_flux:
+    path: /Users/rxlabz/dev/examples/flutter/examples/redux_styles/flutter_flux-1


I think we should remove this absolute path, yes?

it will probably help 😊

Hixie · 2017-08-24T18:16:31Z

analysis_options.yaml

+    enableAssertInitializer: true
+  strong-mode:
+    implicit-dynamic: false
+


we shouldn't need this file, flutter analyze should automatically get the options from the flutter repo.

Hixie · 2017-08-24T18:17:19Z

example/README.md


-1. `cd chat_app`
-2. `flutter run`
+```


Presumably this was unintentional?

Hixie · 2017-08-24T18:17:25Z

example/analysis_options.yaml

+    enableSuperMixins: true
+    enableAssertInitializer: true
+  strong-mode:
+    implicit-dynamic: false


same comment as above

Hixie · 2017-08-24T18:18:10Z

example/android/build.gradle

+
+task clean(type: Delete) {
+    delete rootProject.buildDir
+}


please don't check in the .jar files or the gradlew and gradlew.bat files

Sorry, I'm not sure to understand, isn't it the default generated build.gradle ?

Sorry I put the comment nearest the files in the diff. It wasn't a comment on build.gradle. Github won't let me comment on the actual binary files.

The patch includes the gradle wrapper jar file and the shell scripts. It should not.

Thanks, I removed the gradle dir and gradlew files

Hixie · 2017-08-24T18:19:02Z

example/lib/main.dart

@@ -4,8 +4,7 @@

 import 'package:flutter/material.dart';
 import 'package:flutter_flux/flutter_flux.dart';
-
-import 'stores.dart';
+import 'package:flutter_flux_example/stores.dart';


this shouldn't be imported using package:

Hixie · 2017-08-24T18:19:24Z

example/lib/main.dart

@@ -16,6 +15,8 @@ void main() {
 }

 class ChatScreen extends StoreWatcher {
+  final TextEditingController msgController = new TextEditingController();
+
  ChatScreen({Key key}) : super(key: key);


nit: constructor first

see our style guide for more details on flutter style
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo

Hixie · 2017-08-24T18:19:42Z

example/lib/main.dart

                  padding: new EdgeInsets.symmetric(horizontal: 8.0),
-                  scrollAnchor: ViewportAnchor.end,
+                  /*scrollAnchor: ViewportAnchor.end,*/


don't commit commented out code

Hixie · 2017-08-24T18:20:16Z

lib/src/store_watcher.dart

 import 'package:flutter/widgets.dart';
 import 'package:flutter_flux/src/store.dart';
+import 'package:meta/meta.dart';


import flutter/foundation.dart instead

Hixie · 2017-08-24T18:21:12Z

lib/src/store_watcher.dart

-    if (!mounted)
-      return;
-    setState(() { });
+    if (!mounted) return;


this and the other change below seem to be extraneous and violate the flutter style guide.
there's various other things that could be done in this file to improve its adherence to the style guide.

this 2 changes are only caused by dartfmt. How can I try to improve it , any advice ?

We don't use dartfmt. It doesn't implement Flutter's style guide.

I didn't know that, I restored the original formatting.
So there is no automated way to format to Flutter's style guide ?

Hixie · 2017-08-24T18:21:24Z

lib/src/store_watcher.dart

-      return;
-    setState(() { });
+    if (!mounted) return;
+    setState(() {});


add a comment inside the closure saying what state changed

Hixie · 2017-08-24T18:21:42Z

lib/src/store_watcher.dart

@@ -145,8 +149,7 @@ class StoreToken {

  @override
  bool operator ==(dynamic other) {
-    if (other is! StoreToken)
-      return false;
+    if (other is! StoreToken) return false;


we should really compare runtimeTypes not use is here.

Hixie · 2017-08-24T18:22:02Z

Overall this is great, just some minor issues.

rxlabz · 2017-08-29T16:27:03Z

@Hixie Do I need to do something more, or better ?

Hixie · 2017-08-29T17:26:06Z

lib/src/store_watcher.dart

@@ -85,7 +79,7 @@ class StoreWatcherState extends State<StoreWatcher> with StoreWatcherMixin {
 /// Listens to changes in a number of different stores.
 ///
 /// Used by [StoreWatcher] to track which stores the widget is listening to.
-abstract class StoreWatcherMixin implements State<dynamic> { // ignore: TYPE_ARGUMENT_NOT_MATCHING_BOUNDS, https://github.com/dart-lang/sdk/issues/25232
+abstract class StoreWatcherMixin <T extends StatefulWidget> implements State<T>{


nit: remove space before the <

Hixie · 2017-08-29T17:28:38Z

lib/src/action.dart

@@ -58,7 +58,7 @@ class Action<T> implements Function {
    // a [Stream]-based action implementation. At smaller sample sizes this
    // implementation slows down in comparison, yielding average times of 0.1 ms
    // for stream-based actions vs. 0.14 ms for this action implementation.
-    return Future.wait(_listeners
+    return Future.wait<dynamic>(_listeners
        .map((OnData l) => new Future<dynamic>.microtask(() => l(payload))));
  }


nit: flutter style would be:

return Future.wait<dynamic>( _listeners.map( (OnData listener) => new Future<dynamic>.microtask(() => listener(payload)), ), );

Hixie · 2017-08-29T17:30:21Z

Hixie · 2017-08-29T17:30:52Z

Let me know once you've fixed the two trivial issues I just pointed out and I can land it for you!

Thanks so much for your contribution! And sorry about the delay in responding.

to follow the Flutter style guide

rxlabz · 2017-08-29T21:10:10Z

@Hixie Thanks for the review. the two last issues are fixed now !
( so happy to make my first PR to the Flutter repo ( and to a major OSS project ) ! :) )

Hixie · 2017-08-29T22:14:50Z

Hm, I just noticed the cla bot didn't trigger. Let me look into that before I land it.

Hixie · 2017-08-29T22:59:16Z

Oh never mind, it did trigger, we just don't have the labels set.

Hixie · 2017-08-29T22:59:37Z

Ok, all's good! I'm landing it! Thanks so much for the contribution!

rxlabz and others added 7 commits August 22, 2017 13:55

remove obsolete example

1c72e9e

update to latest Flutter SDK

fa0a9dd

add updated example

96df81c

Update travis.sh

e9f0ed0

Update travis.sh

69bbe91

Tests : replace expectAsync calls by expectAsync1

6b0fde8

Update action_test.dart

1f3e8cb

rxlabz mentioned this pull request Aug 23, 2017

flutter_flux is out of date #13

Closed

sethladd reviewed Aug 23, 2017

View reviewed changes

fix example yaml

400a43b

Hixie reviewed Aug 24, 2017

View reviewed changes

example/README.md Outdated

1. `cd chat_app`

2. `flutter run`

```

Copy link

Contributor

Hixie Aug 24, 2017

Choose a reason for hiding this comment

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

Presumably this was unintentional?

Hixie reviewed Aug 24, 2017

View reviewed changes

rxlabz added 8 commits August 24, 2017 21:15

update the example README

ef1a214

remove analysis_option files

3787e1d

replace package imports by relative paths imports

aac3018

move the constructor to the first line of the class

d48ff1a

replace the meta package import by flutter/foundation

8761b60

compare runtimeType instead of using 'is'

e8427a8

back to the initial Flutter style formatting

b637b19

remove comments and clean the pubspec file

715d765

rxlabz added 2 commits August 25, 2017 10:38

remove the graffle folder and scrips

c4067ca

add iml to gitignore

3c3d826

Hixie reviewed Aug 29, 2017

View reviewed changes

rxlabz added 2 commits August 29, 2017 22:20

remove a superfluous space

1ddeb18

reformat the Action class body

7f63bef

to follow the Flutter style guide

Hixie merged commit 628e178 into google:master Aug 29, 2017

rxlabz deleted the updateToLatestDSK branch September 3, 2017 21:54

This was referenced Oct 7, 2017

Remove flutter.yaml and merge it into pubspec.yaml #10

Closed

Chat example should be updated to current Flutter version #12

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to latest Flutter SDK #14

Update to latest Flutter SDK #14

rxlabz commented Aug 22, 2017

sethladd Aug 23, 2017

rxlabz Aug 23, 2017

Hixie Aug 24, 2017

Hixie Aug 24, 2017

Hixie Aug 24, 2017

Hixie Aug 24, 2017

rxlabz Aug 24, 2017 •

edited

Hixie Aug 24, 2017

rxlabz Aug 25, 2017

Hixie Aug 24, 2017

Hixie Aug 24, 2017

Hixie Aug 24, 2017

Hixie Aug 24, 2017

Hixie Aug 24, 2017

rxlabz Aug 24, 2017

Hixie Aug 24, 2017

rxlabz Aug 25, 2017

Hixie Aug 24, 2017

Hixie Aug 24, 2017

Hixie commented Aug 24, 2017

rxlabz commented Aug 29, 2017

Hixie Aug 29, 2017

Hixie Aug 29, 2017

Hixie commented Aug 29, 2017

Hixie commented Aug 29, 2017

rxlabz commented Aug 29, 2017

Hixie commented Aug 29, 2017

Hixie commented Aug 29, 2017

Hixie commented Aug 29, 2017

Update to latest Flutter SDK #14

Update to latest Flutter SDK #14

Conversation

rxlabz commented Aug 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rxlabz Aug 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hixie commented Aug 24, 2017

rxlabz commented Aug 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hixie commented Aug 29, 2017

Hixie commented Aug 29, 2017

rxlabz commented Aug 29, 2017

Hixie commented Aug 29, 2017

Hixie commented Aug 29, 2017

Hixie commented Aug 29, 2017

rxlabz Aug 24, 2017 •

edited