feat: add icon to AlertDialog by werainkhatri · Pull Request #104920 · flutter/flutter

werainkhatri · 2022-05-29T10:01:41Z

From Spec:

Flutter (MacOS):

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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

werainkhatri · 2022-05-29T10:34:34Z

Material 3 states that the padding between title and content should be 16dp, but currently this padding is 20dp, provided by contentPadding, which is a non-null parameter. Should I turn contentPadding to nullable and set the padding according to if the theme is material 3? or should I keep it non-null and modify it before applying?

packages/flutter/lib/src/material/dialog.dart

packages/flutter/lib/src/material/dialog_theme.dart

guidezpl · 2022-05-30T08:37:50Z

packages/flutter/lib/src/material/dialog.dart

I'd make contentPadding nullable and follow the example that titlePadding sets. Please document similarly, see lines 259-261.

packages/flutter/lib/src/material/dialog.dart

werainkhatri · 2022-05-30T18:35:11Z

if iconColor is not provided:

for M2, it's straightforward, use the Theme.of(context).dialogTheme, else use _DefaultsM2 which uses Theme.of(context).iconTheme (right?)
for M3, the default _TokenDefaultsM3's iconColors is colorScheme.secondary, so Theme.of(context).iconTheme will not be used. is this expected?

packages/flutter/lib/src/material/dialog.dart

werainkhatri · 2022-05-30T19:08:22Z

will fix the test and add more in the next commit :)

packages/flutter/lib/src/material/dialog.dart

guidezpl · 2022-05-31T10:19:28Z

packages/flutter/lib/src/material/dialog.dart

Suggested change

/// If null, [DialogTheme.iconColor] is used.

/// If null, [DialogTheme.iconColor] is used. If that is also null, ..."

packages/flutter/lib/src/material/dialog.dart

guidezpl · 2022-06-03T08:42:13Z

Please resolve conflicts, then I can review

werainkhatri · 2022-06-03T08:44:17Z

Please resolve conflicts, then I can review

I should've merged instead of rebase 😅

I'm away today, will do this over the weekend 😊

guidezpl · 2022-06-08T11:02:40Z

packages/flutter/lib/src/material/dialog.dart

Suggested change

/// of the [icon]. If [title] is _not_ null, an extra 16 pixels of bottom

/// of the [icon]. If [title] is _not_ null, 16 pixels of bottom

Suggested change

/// of the [icon]. If [title] is _not_ null, an extra 16 pixels of bottom

/// of the [icon]. If [title] is _not_ null, an extra 16 pixels of bottom

guidezpl · 2022-06-08T11:04:41Z

While providing padding can be done with the icon, to be consistent with titlePadding and contentPadding, I don't have an issue with including it here. WDYT, @darrenaustin ?

werainkhatri requested review from TahaTesser and darrenaustin May 29, 2022 10:01

flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. labels May 29, 2022

werainkhatri force-pushed the add-icon-to-dialog branch 2 times, most recently from 57bb050 to 9fe0a43 Compare May 29, 2022 10:31

guidezpl requested changes May 30, 2022

View reviewed changes

werainkhatri commented May 30, 2022

View reviewed changes

packages/flutter/lib/src/material/dialog.dart Outdated Show resolved Hide resolved

werainkhatri commented May 30, 2022

View reviewed changes

packages/flutter/lib/src/material/dialog.dart Outdated Show resolved Hide resolved

werainkhatri requested a review from guidezpl May 30, 2022 18:59

guidezpl reviewed May 31, 2022

View reviewed changes

packages/flutter/lib/src/material/dialog.dart Outdated Show resolved Hide resolved

guidezpl reviewed May 31, 2022

View reviewed changes

werainkhatri force-pushed the add-icon-to-dialog branch 3 times, most recently from 004a852 to 7d543e9 Compare June 1, 2022 14:32

werainkhatri marked this pull request as ready for review June 1, 2022 16:10

werainkhatri requested a review from guidezpl June 1, 2022 16:13

werainkhatri commented Jun 1, 2022

View reviewed changes

packages/flutter/lib/src/material/dialog.dart Outdated Show resolved Hide resolved

werainkhatri force-pushed the add-icon-to-dialog branch from 7a0826f to c1dac73 Compare June 1, 2022 16:50

guidezpl reviewed Jun 8, 2022

View reviewed changes

Viren Khatri added 3 commits June 20, 2022 19:21

feat: add icon to AlertDialog

e34834d

refactor: address nits

dd5926c

test: DialogTheme.iconColor

48b4f9e

Viren Khatri and others added 4 commits June 20, 2022 19:21

fix: padding and test: AlertDialog.icon

c0240f4

fix: iconPadding documentation

45d6e84

fix: use IconThemeData instead of ThemeData

c47eca0

doc: removed "an extra"

d1097f6

werainkhatri force-pushed the add-icon-to-dialog branch from 083c07f to d1097f6 Compare June 20, 2022 13:51

guidezpl approved these changes Jun 20, 2022

View reviewed changes

werainkhatri added the waiting for tree to go green label Jun 20, 2022

fluttergithubbot merged commit 672859a into flutter:master Jun 20, 2022

engine-flutter-autoroll mentioned this pull request Jun 20, 2022

Roll Flutter from 02558d69d923 to 672859a0cb79 (21 revisions) flutter/plugins#6005

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jun 20, 2022

672859a feat: add icon to AlertDialog (flutter/flutter#104920)

794fc16

This was referenced Jun 21, 2022

Roll Flutter from 02558d69d923 to d4929bc828ff (23 revisions) flutter/plugins#6008

Closed

Roll Flutter from 4f034a40454b to d4929bc828ff (5 revisions) flutter/packages#2267

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jun 21, 2022

672859a feat: add icon to AlertDialog (flutter/flutter#104920)

0262e42

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 21, 2022

672859a feat: add icon to AlertDialog (flutter/flutter#104920)

1f869c6

engine-flutter-autoroll mentioned this pull request Jun 21, 2022

Roll Flutter from 02558d69d923 to b5c0f8ab9a96 (24 revisions) flutter/plugins#6010

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jun 21, 2022

672859a feat: add icon to AlertDialog (flutter/flutter#104920)

0064dfc

engine-flutter-autoroll mentioned this pull request Jun 21, 2022

Roll Flutter from 02558d69d923 to dfaec11ba519 (29 revisions) flutter/plugins#6011

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jun 21, 2022

672859a feat: add icon to AlertDialog (flutter/flutter#104920)

15d4fb1

camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022

feat: add icon to AlertDialog (flutter#104920)

1c1b0fc

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 30, 2022

672859a feat: add icon to AlertDialog (flutter/flutter#104920)

b0324b7

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 30, 2022

672859a feat: add icon to AlertDialog (flutter/flutter#104920)

c833e38

	/// If null, [DialogTheme.iconColor] is used.
	/// If null, [DialogTheme.iconColor] is used. If that is also null, ..."

	/// of the [icon]. If [title] is _not_ null, an extra 16 pixels of bottom
	/// of the [icon]. If [title] is _not_ null, 16 pixels of bottom

Conversation

werainkhatri commented May 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

werainkhatri commented May 29, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

guidezpl May 30, 2022

Choose a reason for hiding this comment

Uh oh!

werainkhatri May 30, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

werainkhatri commented May 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

werainkhatri commented May 30, 2022

Uh oh!

Uh oh!

guidezpl May 31, 2022

Choose a reason for hiding this comment

Uh oh!

werainkhatri Jun 1, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

guidezpl commented Jun 3, 2022

Uh oh!

werainkhatri commented Jun 3, 2022

Uh oh!

guidezpl Jun 8, 2022

Choose a reason for hiding this comment

Uh oh!

guidezpl commented Jun 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

werainkhatri commented May 29, 2022 •

edited

Loading

werainkhatri commented May 30, 2022 •

edited

Loading