Skip to content

Expose insetPadding and clipBehavior in Dialog and AlertDialog.#50775

Merged
fluttergithubbot merged 4 commits intoflutter:masterfrom
darrenaustin:dialog_padding
Feb 20, 2020
Merged

Expose insetPadding and clipBehavior in Dialog and AlertDialog.#50775
fluttergithubbot merged 4 commits intoflutter:masterfrom
darrenaustin:dialog_padding

Conversation

@darrenaustin
Copy link
Copy Markdown
Contributor

@darrenaustin darrenaustin commented Feb 14, 2020

Description

Currently the Dialog class has a hard coded padding used for the outer part of the dialog. This makes it hard for large dialogs like the Date Picker to use more of the screen. In addition, there is no way to specify the clipBehavior used by the Dialog's internal Material widget. This means that there is no way to have the dialog's content clipped to the given shape, as it defaults to Clip.none.

This PR exposes new insetPadding and clipBehavior parameters to the constructors for the Dialog and AlertDialog classes.

Related Issues

Closes #50757

Tests

I added tests to the dialog_test.dart that tests both of these parameters.

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 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.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

  • No, no existing tests failed, so this is not a breaking change.

@fluttergithubbot fluttergithubbot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Feb 14, 2020
final Curve insetAnimationCurve;

/// {@template flutter.material.dialog.insetPadding}
/// The amount of padding added to the outside of the dialog.
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.

I'm not sure what the right way to describe this is, but I'm assuming this means the padding between the edges of the screen and the dialog itself? I had to think about this for a while and confused between that meaning and the padding between the edges of the dialog and its contents.

Also, do we only see this when the dialog is expanded to be as large as it can be? If I recall correctly, the dialog will be smaller if there the dialog's contents are small enough

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yup. I struggled with that too. I will try come up with a better description.

Comment on lines +56 to +57
}) : assert(clipBehavior != null)
, super(key: key);
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.

I think this is typically formatted this way:

  }) : assert(clipBehavior != null), 
       super(key: key);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I could have sworn I have seen it this way in other files, but looking at the style guidelines again, you are indeed correct. Thx. I will fix that shortly.

Comment thread packages/flutter/lib/src/material/dialog.dart Outdated
Comment on lines +259 to +261
}) : assert(contentPadding != null)
, assert(clipBehavior != null)
, super(key: key);
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.

  }) : assert(contentPadding != null),
       assert(clipBehavior != null),
       super(key: key);

Copy link
Copy Markdown
Contributor

@shihaohong shihaohong left a comment

Choose a reason for hiding this comment

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

LGTM

/// {@template flutter.material.dialog.insetPadding}
/// The amount of padding added to the outside of the dialog.
/// The amount of padding added to [MediaQueryData.viewInsets] on the outside
/// of the dialog. This defines the minimum space between the screen's edges
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.

Nice, this last sentence makes it very clear!

this.backgroundColor,
this.elevation,
this.semanticLabel,
this.insetPadding = const EdgeInsets.symmetric(horizontal: 40.0, vertical: 24.0),
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.

Wondering if this const EdgeInsets should be factored into a constant since its used twice.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Just updated to include this.

@fluttergithubbot fluttergithubbot merged commit 7a83c6f into flutter:master Feb 20, 2020
@darrenaustin darrenaustin deleted the dialog_padding branch February 20, 2020 20:32
darrenaustin added a commit that referenced this pull request Feb 20, 2020
@ycherniavskyi
Copy link
Copy Markdown
Contributor

@darrenaustin could you please clarify why insetPadding and clipBehavior were not added to SimpleDialog as well?

Because if it was unintended (without any actual reason), I could create PR with such addition. I need to have the possibility to reduce the height of SimpleDialog with a large list of SimpleDialogOption, and it could be achieved by insetPadding.

@darrenaustin
Copy link
Copy Markdown
Contributor Author

@ycherniavskyi This was just an oversight. A PR to add them to SimpleDialog would be welcome. Thanks.

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.

Content inside Dialog widget does not get clipped when necessary

6 participants