-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with just a few comments
@@ -86,7 +86,7 @@ class HomePage extends StatelessWidget { | |||
package: 'flutter_gallery_assets', | |||
), | |||
assetDarkColor: const Color(0xFF253538), | |||
studyRoute: RallyApp.loginRoute, | |||
studyRoute: rally_routes.loginRoute, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why the Shrine
and StarterApp
routes aren't changed to do the same as the other studies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
lib/pages/home.dart
Outdated
@@ -1118,6 +1118,41 @@ double _carouselHeight(double scaleFactor, BuildContext context) => math.max( | |||
scaleFactor, | |||
_carouselHeightMin); | |||
|
|||
typedef LibraryLoader = Future<void> Function(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might make more sense for the LibraryLoader and DeferredWidget to be moved into its own dart file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx for suggestion. Moved to deferred_widget.dart
lib/studies/crane/routes.dart
Outdated
@@ -0,0 +1 @@ | |||
const String defaultRoute = '/crane'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: needs new line at end of file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
lib/studies/fortnightly/routes.dart
Outdated
@@ -0,0 +1 @@ | |||
const String defaultRoute = '/fortnightly'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: needs new line at end of file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -0,0 +1,2 @@ | |||
const String loginRoute = '/rally/login'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the route consts for all the studies just be in one file instead of multiple?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Torn on this one. I like that they are pretty independent. the demo types are breaking that right now, maybe those should be split too.
@@ -0,0 +1,2 @@ | |||
const String loginRoute = '/shrine/login'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this file is missing copyright header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one comment
@@ -86,7 +86,7 @@ class HomePage extends StatelessWidget { | |||
package: 'flutter_gallery_assets', | |||
), | |||
assetDarkColor: const Color(0xFF253538), | |||
studyRoute: RallyApp.loginRoute, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the Shrine and Starter app are not using the new routes files you added yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
This drops release binary size from 4,936,531 to 4,125,088.