Skip to content

Conversation

@ronnnnn
Copy link
Contributor

@ronnnnn ronnnnn commented Jul 26, 2023

Overview

Add barrierDismissible, barrierColor and barrierLabel parameters to showDatePicker, showTimePicker and showAboutDialog which calls showDialog internally. We can change these parameters with showDialog and Dialog widgets (like DatePickerDialog, TimePickerDialog or AboutDialog) directly. But, I think it is prefer to provide interfaces same as showDialog to keep application wide unified looks if it is used internally.

Fixes #130971

(cherry picked from commit 9def8f6)

Context

#130484 was reverted as it caused postsubmit test failures.
I pulled latest master changes and cherry-picked 9def8f6.

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • All existing and new tests are passing.

…d showAboutDialog. (#130484)

### Overview

Add `barrierDismissible`, `barrierColor` and `barrierLabel` parameters to  `showDatePicker`, `showTimePicker` and `showAboutDialog` which calls `showDialog` internally.
We can change these parameters with `showDialog` and Dialog widgets (like `DatePickerDialog`, `TimePickerDialog` or `AboutDialog`) directly. But, I think it is prefer to provide interfaces same as `showDialog` to keep application wide unified looks if it is used internally.

Fixes #130971

(cherry picked from commit 9def8f6)
@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Jul 26, 2023
@ronnnnn
Copy link
Contributor Author

ronnnnn commented Jul 26, 2023

@HansMuller @justinmc

I pulled latest master changes and cherry-picked 9def8f6.
And I retried failed test as @loic-sharma mentioned and it passed right on my local.

Any idea about it?

@ronnnnn
Copy link
Contributor Author

ronnnnn commented Jul 26, 2023

I didn't face the error on my local environment but the test failed as mentioned on CI.

I fixed with not to use setupAll to init NavigatorObserver then test passed correctly. ebd965b
But I didn't understand why the difference of behavior between CI and loacl environments happened.

Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

LGTM 👍 . Thanks for following up on this after it was reverted!

@justinmc
Copy link
Contributor

I would guess that the CI environment is running the tests in parallel, or in a different order, or something like that... Not sure though.

@ronnnnn
Copy link
Contributor Author

ronnnnn commented Jul 27, 2023

@justinmc
Thank you for your advice.
I misunderstood behavior of setUpAll.
The test was flaky with using setUpAll and test concurrency as you commented, I guess.

I replaced with setUp to init NavigatorObserver before each tests. 4339e5a

@ronnnnn ronnnnn requested a review from justinmc July 27, 2023 11:08
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

LGTM 👍. Thanks for changing it to setUp. I guess the name setUpAll is confusing now that I think about it...

There was one test failing but I think it was an infrastructure flake, I've rerun it.

@justinmc
Copy link
Contributor

@HansMuller for secondary review since you already reviewed the original PR #130484.

Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

LGTM

@justinmc justinmc merged commit dd9764e into flutter:master Jul 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 27, 2023
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jul 27, 2023
flutter/flutter@61fd11d...dd9764e

2023-07-27 [email protected] Proposal to add barrier configs for showDatePicker, showTimePicker and showAboutDialog. (flutter/flutter#131306)
2023-07-27 [email protected] Fix ios_add2app Podfile (flutter/flutter#131263)
2023-07-27 [email protected] Add DeviceLab build+test separation doc (flutter/flutter#131365)
2023-07-27 [email protected] IgnoreBaseline widget (flutter/flutter#131220)
2023-07-27 [email protected] Add 'vm:keep-name' pragmas to platform channel classes (flutter/flutter#131271)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@ronnnnn ronnnnn deleted the feat-barrier-configs-param-for-showXxx branch July 28, 2023 00:37
LouiseHsu pushed a commit to LouiseHsu/flutter that referenced this pull request Jul 31, 2023
…d showAboutDialog. (flutter#131306)

Can configure modal barriers in Flutter's built-in dialogs.
vashworth pushed a commit to vashworth/flutter that referenced this pull request Aug 2, 2023
…d showAboutDialog. (flutter#131306)

Can configure modal barriers in Flutter's built-in dialogs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Custom barrier color with showDatePicker, showTimePicker and showAboutDialog

3 participants