-
Notifications
You must be signed in to change notification settings - Fork 5
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
miscellaneous updates to the current_results_ui codebase #125
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. I've copied this PR over to the Gerrit CL
https://dart-review.googlesource.com/c/dart_ci/+/233780
and added some changes corresponding to my comments.
The resulting CL is deployed to staging, for testing, at
https://dart-ci-staging.firebaseapp.com/current_results
I have filed the question about adding Contributors information,
and/or switching to Github reviews, as a new issue: #127
@@ -8,7 +8,6 @@ environment: | |||
sdk: ">=2.9.0 <3.0.0" | |||
|
|||
dependencies: | |||
clippy: ^1.0.0 |
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.
The pubspec.lock file is checked in, so generate it with a specified version of Flutter, and commit the changes.
@@ -75,7 +75,7 @@ class Providers extends StatelessWidget { | |||
child: Builder( | |||
// ChangeNotifierProvider.value in a Builder is needed to make | |||
// the TabController available for widgets to observe. | |||
builder: (context) => ChangeNotifierProvider<TabController>.value( | |||
builder: (context) => ChangeNotifierProvider<TabController?>.value( | |||
value: DefaultTabController.of(context), |
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.
I prefer to add ! to the argument, here.
@@ -48,8 +48,8 @@ class ResultsPanel extends StatelessWidget { | |||
|
|||
class ExpandableResult extends StatefulWidget { | |||
final String name; | |||
final Map<ChangeInResult, List<Result>> changeGroups; | |||
final Counts counts; | |||
final Map<ChangeInResult, List<Result>>? changeGroups; |
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.
I prefer to make these non-nullable, and add null asserts to the values before constructing.
@@ -60,11 +60,11 @@ class ExpandableResult extends StatefulWidget { | |||
|
|||
class CountItem { | |||
final String text; | |||
final Color color; | |||
final Color? color; |
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.
I prefer to make this non-null.
|
||
CountItem._(this.text, this.color); | ||
|
||
factory CountItem(int count, Color color) { | ||
factory CountItem(int count, Color? color) { |
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.
I prefer to add non-null assertions in lines 80-82.
@@ -139,7 +139,7 @@ class _ExpandableResultState extends State<ExpandableResult> { | |||
), | |||
), | |||
if (expanded) | |||
for (final change in changeGroups.keys) | |||
for (final change in changeGroups!.keys) |
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.
This could be non-nullable, as suggested above.
To remove the null checks on changeGroups[change] below, we would need to iterate over
changeGroups.entries, which is perhaps too ugly.
@@ -250,7 +250,7 @@ class Summary extends StatelessWidget { | |||
} | |||
|
|||
class Pill extends StatelessWidget { | |||
final Color color; | |||
final Color? color; |
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.
I prefer non-nullable.
@@ -28,7 +28,7 @@ class CurrentResultsApp extends StatelessWidget { | |||
), | |||
initialRoute: '/', | |||
onGenerateRoute: (RouteSettings settings) { | |||
final parameters = settings.name.substring(1).split('&'); | |||
final parameters = settings.name!.substring(1).split('&'); | |||
|
|||
final terms = parameters | |||
.firstWhere((parameter) => parameter.startsWith('filter='), |
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.
In line 49, I had to add ! to WidgetsBinding.instance to avoid an analyzer error with Flutter 2.11.0-0.1.pre
Thanks for moving this to a gerrit review! I lgtm'd it there. |
This is a copy of PR #125 by Devon Carew (devoncarew). Drop web-only packages and use flutter_lints. Specify the deployment Flutter version in README.md. Change-Id: Ic81b7910c725b6f0cdb28bea94e3c78489f824f1 Reviewed-on: https://dart-review.googlesource.com/c/dart_ci/+/233780 Reviewed-by: Devon Carew <devoncarew@google.com> Commit-Queue: William Hesse <whesse@google.com>
Various and sundry updates to the current_results_ui/ codease:
dart fix
dart migrate
--
I see this in the readme.md:
If PR won't work here, I can recreate this work as a gerrit CL. However, it would be nice to either take PRs, or, add some brief contributing instructions to the repo to describe how to author changes.
You might consider flipping the system of record for the codebase, as that would allow us to do things like use github actions to validate the code.