Skip to content

Conversation

@domesticmouse
Copy link
Contributor

Pre-launch Checklist

  • I read the Effective Dart: Style recently, and have followed its advice.
  • I signed the CLA.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-devrel channel on Discord.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request updates dependencies across multiple steps of the firebase-get-to-know-flutter codelab. The main changes involve updating go_router and provider packages in the pubspec.yaml files, and regenerating the corresponding iOS and macOS project files. The changes look good overall. I've added a few suggestions to sort the dependencies in the pubspec.yaml files alphabetically. This improves readability and aligns with the sort_pub_dependencies lint rule often used with flutter_lints, which is recommended in your style guide.

Copy link
Member

@parlough parlough left a comment

Choose a reason for hiding this comment

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

Looks good to me, but one small regression and a few links that could be updated:

network to communicate with the Firebase servers, you need to configure your app
with network client privileges.

#### [macos/Runner/DebugProfile.entitlements](https://github.com/flutter/codelabs/blob/master/firebase-get-to-know-flutter/step_04/macos/Runner/DebugProfile.entitlements)
Copy link
Member

Choose a reason for hiding this comment

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

It seems this and the other flutter/codelabs link in the doc were changed to reference master instead of main. Since the repository uses main, can you undo that change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hooboy, good catch. The published codelab is still running with links to master.

domesticmouse and others added 5 commits August 28, 2025 16:01
Co-authored-by: Parker Lougheed <parlough@gmail.com>
Co-authored-by: Parker Lougheed <parlough@gmail.com>
Co-authored-by: Parker Lougheed <parlough@gmail.com>
Co-authored-by: Parker Lougheed <parlough@gmail.com>
Theme.of(context).textTheme,
),
visualDensity: VisualDensity.adaptivePlatformDensity,
useMaterial3: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Revert this

});
// ...to here.
} else {
_loginState = ApplicationLoginState.loggedOut;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might be a reversion.

@domesticmouse
Copy link
Contributor Author

domesticmouse commented Sep 25, 2025

Landing this as is so that it matches what has just been published on codelabs.developers.google.com

@domesticmouse domesticmouse merged commit b7aa22c into flutter:main Sep 25, 2025
10 checks passed
@domesticmouse domesticmouse deleted the Update-firebase-get-to-know-flutter branch September 25, 2025 23:39
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