-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Fix Material 3 AppBar.leading action IconButtons
#154512
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.
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
Fix Material 3 AppBar.leading action IconButtons
#154512
Conversation
0317dac to
26e7792
Compare
| final bool isIconButton = leading is IconButton | ||
| || leading is CloseButton | ||
| || leading is BackButton | ||
| || leading is DrawerButton; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a better way to detect these additional buttons use IconButton underneath?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we refactor _ActionButton to extends IconButton? Seems _ActionButton just returns a IconButton
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While exploring this suggestion I realized i also missed EndDrawerButton in this check. This really goes to show this type of fix is not very robust.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored the _ActionButton to extend IconButton and reverted the updated check
QuncCccccc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
| final bool isIconButton = leading is IconButton | ||
| || leading is CloseButton | ||
| || leading is BackButton | ||
| || leading is DrawerButton; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we refactor _ActionButton to extends IconButton? Seems _ActionButton just returns a IconButton
26e7792 to
034848b
Compare
c4d8555 to
7df4e32
Compare
|
@QuncCccccc this seems to be failing Google testing |
I'm working on it!😎 Update: sent cl/671169268 and cl/671527109 for review |
|
@QuncCccccc thanks for helping! |
Fixes
AppBarback button focus/hover circle should not fill up whole heightFixes [Material 3] Date Range Picker close button has incorrect shape
This updates the leading condition added in #110722
Code sample
expand to view the code sample
Before
After
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.