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

Disable DevTools pages based on the connected app type. #1757

Merged
merged 10 commits into from
Mar 23, 2020

Conversation

kenzieschmoll
Copy link
Member

When the page is not supported for the connected app, we show a full screen message explaining why the page is disabled.

Screen Shot 2020-03-20 at 10 40 00 AM

Fixes #1669

Copy link
Member

@devoncarew devoncarew left a comment

Choose a reason for hiding this comment

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

Some comments, mostly about possible refactorings around enabled() and the build() methods.

@@ -392,3 +392,57 @@ class OutlinedBorder extends StatelessWidget {
///
/// Makes for nice-looking rectangles.
final goldenRatio = 1 + sqrt(5) / 2;

class DisabledForProfileModeMessage extends StatelessWidget {
Copy link
Member

Choose a reason for hiding this comment

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

👍 I like the individual, specific messages telling why the page is disabled.

bool get isRunningOnDartVM => serviceManager.vm.name != 'ChromeDebugProxy';

Future<bool> get isDartCliApp async =>
FutureOr<bool> get isDartCliApp async =>
Copy link
Member

Choose a reason for hiding this comment

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

I think Future<bool> is correct here.

Copy link
Member Author

Choose a reason for hiding this comment

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

since isFlutterApp returns a FutureOr, wouldn't it be FutureOr?

Copy link
Member

Choose a reason for hiding this comment

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

I think they can both be changed to Future?

It's not a critical change.

style: Theme.of(context).textTheme.headline4,
),
),
child: CenteredMessage('Sorry, $uri was not found.'),
Copy link
Member

Choose a reason for hiding this comment

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

We should definitely include a version of the chrome 404 dinosaur game here: https://lmgtfy.com/?q=chrome+404+dinosaur+game.

NetworkScreen screen;
FakeServiceManager fakeServiceManager;

group('NetworkScreen', () {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice tests!

Copy link
Contributor

@jacob314 jacob314 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! Some pretty minor comments.

@kenzieschmoll kenzieschmoll merged commit 143c853 into flutter:master Mar 23, 2020
@kenzieschmoll kenzieschmoll deleted the disablePages branch March 23, 2020 22:13
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.

Detect app type and mode and disable proper devtools pages
3 participants