Skip to content
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

Migrate theme.dart to be null safe. #3633

Merged
merged 43 commits into from
Feb 8, 2022
Merged

Migrate theme.dart to be null safe. #3633

merged 43 commits into from
Feb 8, 2022

Conversation

polina-c
Copy link
Contributor

@polina-c polina-c commented Feb 5, 2022

No description provided.

@polina-c polina-c marked this pull request as ready for review February 7, 2022 21:39
@polina-c polina-c changed the title Migrate some libraries to be null safe (2) Migrate theme.dart to be null safe (2) Feb 7, 2022
@polina-c polina-c changed the title Migrate theme.dart to be null safe (2) Migrate theme.dart to be null safe. Feb 7, 2022
@polina-c polina-c marked this pull request as draft February 7, 2022 22:08
@polina-c polina-c marked this pull request as ready for review February 7, 2022 22:37
@@ -12,17 +12,6 @@ import 'package:flutter_test/flutter_test.dart';
void main() {
group('Theme', () {
ThemeData theme;
test('can be used without override', () {
theme = themeFor(isDarkTheme: true, ideTheme: null);
Copy link
Member

Choose a reason for hiding this comment

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

instead of removing this test, why don't we just replace null with IdeTheme()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The goal of the test is to verify everything works with null. We do not need this validation any more.
Other tests here cover non-null cases. Makes sense?

Copy link
Member

Choose a reason for hiding this comment

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

In this case an empty IdeTheme would be the equivalent of what used to be null, in terms of testing the "without override" case. The "without override" case is where a user runs DevTools via dart devtools instead of using the DevTools embedded in their IDE. So we still want to test the case where IdeTheme does not contain overrides for backgroundColor, foregroundColor, etc.

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

@polina-c polina-c marked this pull request as draft February 8, 2022 22:48
@polina-c polina-c marked this pull request as ready for review February 8, 2022 23:11
@polina-c polina-c merged commit 09e79f7 into flutter:master Feb 8, 2022
@polina-c polina-c deleted the ns2 branch February 8, 2022 23:29
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.

2 participants