Skip to content

feat: add icon to AlertDialog #104920

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
fluttergithubbot merged 7 commits into from
Jun 20, 2022
Merged

feat: add icon to AlertDialog #104920

fluttergithubbot merged 7 commits into from
Jun 20, 2022

Conversation

@werainkhatri
Copy link
Member

@werainkhatri werainkhatri commented May 29, 2022 •
edited
Loading

From Spec:

Flutter (MacOS):

Fixes #104882

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.

@flutter-dashboard 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 werainkhatri force-pushed the add-icon-to-dialog branch 2 times, most recently from 57bb050 to 9fe0a43 Compare May 29, 2022 10:31
@werainkhatri
Copy link
Member Author

werainkhatri commented May 29, 2022

Screenshot 2022-05-29 at 3 57 03 PM

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?

guidezpl
guidezpl requested changes May 30, 2022
View reviewed changes
packages/flutter/lib/src/material/dialog.dart Outdated
Copy link
Member

@guidezpl guidezpl May 30, 2022

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@werainkhatri werainkhatri May 30, 2022

Choose a reason for hiding this comment

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

PTAL

@werainkhatri
Copy link
Member Author

werainkhatri commented May 30, 2022 •
edited
Loading

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?

werainkhatri
werainkhatri commented May 30, 2022
View reviewed changes
werainkhatri
werainkhatri commented May 30, 2022
View reviewed changes
@werainkhatri werainkhatri requested a review from guidezpl May 30, 2022 18:59
@werainkhatri
Copy link
Member Author

werainkhatri commented May 30, 2022

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

guidezpl
guidezpl reviewed May 31, 2022
View reviewed changes
guidezpl
guidezpl reviewed May 31, 2022
View reviewed changes
packages/flutter/lib/src/material/dialog.dart Outdated
Copy link
Member

@guidezpl guidezpl May 31, 2022

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@werainkhatri werainkhatri Jun 1, 2022

Choose a reason for hiding this comment

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

PTAL

@werainkhatri werainkhatri force-pushed the add-icon-to-dialog branch 3 times, most recently from 004a852 to 7d543e9 Compare June 1, 2022 14:32
@werainkhatri werainkhatri marked this pull request as ready for review June 1, 2022 16:10
@werainkhatri werainkhatri requested a review from guidezpl June 1, 2022 16:13
werainkhatri
werainkhatri commented Jun 1, 2022
View reviewed changes
@guidezpl
Copy link
Member

guidezpl commented Jun 3, 2022

Please resolve conflicts, then I can review

@werainkhatri
Copy link
Member Author

werainkhatri commented Jun 3, 2022

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
guidezpl reviewed Jun 8, 2022
View reviewed changes
packages/flutter/lib/src/material/dialog.dart Outdated
Copy link
Member

@guidezpl guidezpl Jun 8, 2022

Choose a reason for hiding this comment

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

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

guidezpl commented Jun 8, 2022

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 ?

@fluttergithubbot fluttergithubbot merged commit 672859a into flutter:master Jun 20, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 20, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jun 20, 2022
@engine-flutter-autoroll
672859a feat: add icon to AlertDialog (flutter/flutter#104920)
794fc16
This was referenced Jun 21, 2022
Closed
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jun 21, 2022
@engine-flutter-autoroll
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
@engine-flutter-autoroll
672859a feat: add icon to AlertDialog (flutter/flutter#104920)
1f869c6
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 21, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jun 21, 2022
@engine-flutter-autoroll
672859a feat: add icon to AlertDialog (flutter/flutter#104920)
0064dfc
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 21, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jun 21, 2022
@engine-flutter-autoroll
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
@werainkhatri @camsim99
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
@engine-flutter-autoroll
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
@engine-flutter-autoroll
672859a feat: add icon to AlertDialog (flutter/flutter#104920)
c833e38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@guidezpl guidezpl guidezpl approved these changes

@darrenaustin darrenaustin Awaiting requested review from darrenaustin

@TahaTesser TahaTesser Awaiting requested review from TahaTesser

Assignees

No one assigned

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt. 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.

Material 3 dialog with hero icon

3 participants

@werainkhatri @guidezpl @fluttergithubbot