migrate some material files to nullsafety #67078
Conversation
gspencergoog
left a comment
There was a problem hiding this comment.
Modulo the one comment. Thanks!
|
Can you rebase this perform submitting it? I recently added a field to MaterialApp ( Sorry for the inconvenience... |
318a5d7 to
ec27174
Compare
|
Rebased. |
| showSelectedLabels: showSelectedLabels ?? bottomTheme.showUnselectedLabels, | ||
| showUnselectedLabels: showUnselectedLabels ?? bottomTheme.showUnselectedLabels, | ||
| selectedLabelStyle: selectedLabelStyle, | ||
| unselectedLabelStyle: unselectedLabelStyle, |
There was a problem hiding this comment.
fyi @HansMuller not sure if this was the intended design but this does match what we had before. Maybe we should make selectedLabelStyle and unselectedLabelStyle nullable in some future PR?
There was a problem hiding this comment.
This looks like an obvious error for which I assume we had no test coverage. This widget is supposed to be "conventional", non-null style parameters override defaults valued computed by the widget.
CC @johnsonmh
There was a problem hiding this comment.
Thanks for the catch, this PR should fix the bug and make desired nullability more clear: #67342
| flex: _evaluateFlex(_animations[i]), | ||
| selected: i == widget.currentIndex, | ||
| showSelectedLabels: widget.showSelectedLabels ?? bottomTheme.showSelectedLabels, | ||
| showSelectedLabels: widget.showSelectedLabels, |
| const BottomSheet({ | ||
| Key key, | ||
| Key? key, | ||
| this.animationController, |
There was a problem hiding this comment.
looks like this should be required
There was a problem hiding this comment.
But there are place where no animationController is passed. And sometimes a nullable value is passed as well.
Hixie
left a comment
There was a problem hiding this comment.
Some minor comments but overall LGTM.
I suspect we'll have to do some cleanup later, for example making Theme.of non-nullable, but that will require deeper work (e.g. creating a migration guide, deprecations, etc) which aren't appropriate here.
This reverts commit 8143992.
| } | ||
|
|
||
| Future<void> close() { | ||
| Future<void> close() async { |
There was a problem hiding this comment.
This change (making close() async) caused the test failures in google3 because it's changing the timing in tests slightly. I am going to make this non-async again to restore the old behavior.
Description
NNBD migration for part of material
Related Issues
Tests
I added the following tests:
None
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Did any tests fail when you ran them? Please read Handling breaking changes.