Skip to content

Proposal to add barrier configs for showDatePicker, showTimePicker and showAboutDialog. - #131306

Merged
justinmc merged 7 commits into
flutter:masterfrom
ronnnnn:feat-barrier-configs-param-for-showXxx
Jul 27, 2023
Merged

justinmc merged 7 commits into
flutter:masterfrom
ronnnnn:feat-barrier-configs-param-for-showXxx

Conversation

@ronnnnn

@ronnnnn ronnnnn commented Jul 26, 2023

Copy link
Copy Markdown
Contributor

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. p: material_ui material_ui package in flutter/packages labels Jul 26, 2023
@ronnnnn

ronnnnn commented Jul 26, 2023

Copy link
Copy Markdown
Contributor Author

@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

ronnnnn commented Jul 26, 2023 •

Copy link
Copy Markdown
Contributor Author

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.

@justinmc justinmc left a comment

Copy link
Copy Markdown
Contributor

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!

Comment thread packages/flutter/test/material/about_test.dart Outdated
@justinmc

Copy link
Copy Markdown
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

ronnnnn commented Jul 27, 2023

Copy link
Copy Markdown
Contributor Author

@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

@justinmc justinmc left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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

@HansMuller HansMuller left a comment

Copy link
Copy Markdown
Contributor

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.
creatorpiyush pushed a commit to creatorpiyush/packages that referenced this pull request Jun 10, 2026
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
bisor0627 pushed a commit to bisor0627/packages that referenced this pull request Jun 19, 2026
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

framework flutter/packages/flutter repository. See also f: labels. p: material_ui material_ui package in flutter/packages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Custom barrier color with showDatePicker, showTimePicker and showAboutDialog

3 participants