Skip to content

Implement barrierDismissible for showCupertinoDialog#50915

Merged
fluttergithubbot merged 1 commit intoflutter:masterfrom
tottomotto:master
Apr 29, 2020
Merged

Implement barrierDismissible for showCupertinoDialog#50915
fluttergithubbot merged 1 commit intoflutter:masterfrom
tottomotto:master

Conversation

@tottomotto
Copy link
Contributor

@tottomotto tottomotto commented Feb 17, 2020

Description

Adds support for dismissing cupertino dialog with barrier behavior. By default the dialog is not dismissible with a tap on the barrier. The localization files updates are based on the existing material ones.

Related Issues

Fixes #45744

Tests

I added the following tests:

  • check if barrierDismissible is disabled by default
  • check if barrierDismissible is enabled if enabled explicitly
  • check for existence of the localized modalBarrierDismissLabel for cupertino

Checklist

  • 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 read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • 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.

@fluttergithubbot fluttergithubbot added a: internationalization Supporting other languages or locales. (aka i18n) f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Feb 17, 2020
@HansMuller HansMuller removed their request for review February 21, 2020 15:16
@Piinks
Copy link
Contributor

Piinks commented Mar 18, 2020

Hi @tottomotto welcome! 🎉 Thank you for the contribution.
It looks like there are merge conflicts with this PR. Can you resolve them and update your branch?

xster
xster reviewed Mar 21, 2020
View reviewed changes
packages/flutter/test/cupertino/dialog_test.dart Outdated
Copy link
Member

@xster xster Mar 21, 2020

Choose a reason for hiding this comment

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

Use complete sentences. Also if there are 2 things being tested, prefer adding 2 tests for 'Not barrier dismissible by default' and 'Configurable to be barrier dismissible'.

xster
xster reviewed Mar 21, 2020
View reviewed changes
xster
xster reviewed Mar 21, 2020
View reviewed changes
packages/flutter_localizations/lib/src/l10n/cupertino_en.arb Outdated
Copy link
Member

@xster xster Mar 21, 2020

Choose a reason for hiding this comment

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

can, for example, be

Copy link
Member

@xster xster Mar 21, 2020

Choose a reason for hiding this comment

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

nit: wrap line at 100 characters.

Copy link
Contributor Author

@tottomotto tottomotto Mar 22, 2020

Choose a reason for hiding this comment

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

I'm not sure what you mean by wrap :)

xster
xster reviewed Mar 21, 2020
View reviewed changes
xster
xster reviewed Mar 21, 2020
View reviewed changes
Copy link
Member

@xster xster left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!
LGTM minus a few nits.

heads up @HansMuller in case you run the translation console anytime soon, otherwise I'll run it after this goes in

@tottomotto tottomotto requested a review from xster March 25, 2020 12:41
xster
xster reviewed Mar 31, 2020
View reviewed changes
@xster
Copy link
Member

xster commented Apr 1, 2020

some tests are failing (but not due to any issues in your PR). Could you rebase your PR on head and push again?

@tottomotto tottomotto force-pushed the master branch 2 times, most recently from ff19fe5 to 72e2508 Compare April 2, 2020 11:58
@fluttergithubbot fluttergithubbot merged commit 1d0999d into flutter:master Apr 29, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@xster xster xster approved these changes

Assignees

No one assigned

Labels

a: internationalization Supporting other languages or locales. (aka i18n) f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

showCupertinoDialog should support barrierDismissible

5 participants

@tottomotto @Piinks @xster @googlebot @fluttergithubbot