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

Pad CupertinoAlertDialog with MediaQuery viewInsets #42967

Merged
merged 3 commits into from Nov 5, 2019

Conversation

@edman
Copy link
Member

edman commented Oct 17, 2019

Description

Material's AlertDialog animates a dialog padding as MediaQuery.viewInsets changes. CupertinoAlertDialog does not do the same, currently opening the keyboard while a CupertinoAlertDialog is displayed will cover it.

This PR makes CupertinoAlertDialog aware of MediaQuery.viewInsets by animating the dialog padding to match the current insets, similar to how AlertDialog implements it.

Before:

Screenshot_20191018-000106

After:

Screenshot_20191018-000134

Related Issues

Fixes #42049.

Tests

I intend to add the following tests:

  • Dialog is padded with viewInsets of enclosing MediaQuery.
  • Dialog animates when viewInsets changes.

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.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
  • No, this is not a breaking change.
@edman

This comment has been minimized.

Copy link
Member Author

edman commented Oct 17, 2019

Hey @justinmc, I got this working but having trouble writing tests. It seems the dialog has an alignment somewhere that messes with its final position.

AlertDialog didn't have it, so it was easier to infer the expected position given some viewInsets.

Is there a way to debug a widget test? Like can I inspect the widget tree or see the widgets rendered?

@justinmc

This comment has been minimized.

Copy link
Contributor

justinmc commented Oct 17, 2019

There is no way to view the widgets that a test is pumping (as far as I know). What I usually do is just copy the code into an empty app and manually go through the steps that the test does. You can also put print statements in the Flutter source code and they will be output with the test output.

Two not so nice solutions haha, but hopefully that helps. I can also take a closer look tomorrow.

@edman edman force-pushed the edman:dialog-insets branch from d3f26e1 to 0905fb0 Oct 31, 2019
@edman

This comment has been minimized.

Copy link
Member Author

edman commented Oct 31, 2019

This PR is ready for review.

Copy link
Contributor

justinmc left a comment

Quick comment below about deduplicating docs. Otherwise I think this looks great. I confirmed that the test covers the problem and fails on master. Let me know about the docs thing and I'll approve and then this can be merged!

edman added 3 commits Oct 17, 2019
@edman edman force-pushed the edman:dialog-insets branch from 0905fb0 to 01f6371 Nov 2, 2019
Copy link
Contributor

justinmc left a comment

LGTM, thanks for the docs fix.

I reran a test that seems to be a flake. If this goes green then feel free to merge!

@edman edman merged commit 28b5cc3 into flutter:master Nov 5, 2019
62 of 63 checks passed
62 of 63 checks passed
flutter-build Flutter build is currently broken. Please do not merge this PR unless it contains a fix to the broken build.
Details
WIP Ready for review
Details
analyze-linux Task Summary
Details
analyze-linux
Details
build_tests-linux Task Summary
Details
build_tests-linux
Details
cla/google All necessary CLAs are signed
customer_testing-linux Task Summary
Details
customer_testing-linux
Details
customer_testing-macos Task Summary
Details
customer_testing-macos
Details
customer_testing-windows Task Summary
Details
customer_testing-windows
Details
deploy_gallery-linux Task Summary
Details
deploy_gallery-linux
Details
deploy_gallery-macos Task Summary
Details
deploy_gallery-macos
Details
docs-linux Task Summary
Details
docs-linux
Details
firebase_test_lab_tests-linux Task Summary
Details
firebase_test_lab_tests-linux
Details
framework_tests-libraries-linux Task Summary
Details
framework_tests-libraries-linux
Details
framework_tests-libraries-macos Task Summary
Details
framework_tests-libraries-macos
Details
framework_tests-libraries-windows Task Summary
Details
framework_tests-libraries-windows
Details
framework_tests-misc-linux Task Summary
Details
framework_tests-misc-linux
Details
framework_tests-misc-macos Task Summary
Details
framework_tests-misc-macos
Details
framework_tests-misc-windows Task Summary
Details
framework_tests-misc-windows
Details
framework_tests-widgets-linux Task Summary
Details
framework_tests-widgets-linux
Details
framework_tests-widgets-macos Task Summary
Details
framework_tests-widgets-macos
Details
framework_tests-widgets-windows Task Summary
Details
framework_tests-widgets-windows
Details
hostonly_devicelab_tests-0-linux Task Summary
Details
hostonly_devicelab_tests-0-linux
Details
hostonly_devicelab_tests-1-linux Task Summary
Details
hostonly_devicelab_tests-1-linux
Details
hostonly_devicelab_tests-2-linux Task Summary
Details
hostonly_devicelab_tests-2-linux
Details
hostonly_devicelab_tests-3_last-linux Task Summary
Details
hostonly_devicelab_tests-3_last-linux
Details
web_tests-0-linux Task Summary
Details
web_tests-0-linux
Details
web_tests-1-linux Task Summary
Details
web_tests-1-linux
Details
web_tests-2-linux Task Summary
Details
web_tests-2-linux
Details
web_tests-3-linux Task Summary
Details
web_tests-3-linux
Details
web_tests-4-linux Task Summary
Details
web_tests-4-linux
Details
web_tests-5-linux Task Summary
Details
web_tests-5-linux
Details
web_tests-6-linux Task Summary
Details
web_tests-6-linux
Details
web_tests-7_last-linux Task Summary
Details
web_tests-7_last-linux
Details
sahandevs added a commit to sahandevs/flutter that referenced this pull request Nov 15, 2019
sahandevs added a commit to sahandevs/flutter that referenced this pull request Nov 15, 2019
sahandevs added a commit to sahandevs/flutter that referenced this pull request Nov 15, 2019
Inconnu08 added a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.