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
Migrate IconButton
to Material 3 - Part 2
#106437
Migrate IconButton
to Material 3 - Part 2
#106437
Conversation
IconButton
to Material 3 - Part 2
1fb49cc
to
b14a273
Compare
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.
Overall this looks good. Nice work.
I have some comments and questions below.
// Desktop and web platforms have a compact visual density by default. | ||
// To see buttons with circular background on desktop/web, the "visualDensity" | ||
// needs to be set to "VisualDensity.standard". | ||
visualDensity: VisualDensity.standard, |
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.
This is fine for this PR, but thinking more about this, perhaps we should have the IconButton override the density that is used by ButtonStyledButton to always use standard, as I am not sure it makes sense to apply density to icon buttons. We should talk with @gspencergoog about this.
StandardIconButtons(), | ||
FilledIconButtons(), | ||
FilledTonalIconButtons(), | ||
OutlinedIconButtons(), |
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.
There is a lot of common code for each of these button types. In fact it looks like the only difference is the ButtonStyle. Perhaps it would be simpler to have a single DemoIconToggleButton class that you pass in different ButtonStyles to instead of multiple classes doing the same thing.
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.
I see. Thanks for the suggestion! I added the change. Please let me know if there's any questions:)
b14a273
to
92b0d01
Compare
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. Nice work. Just a minor doc update below. Thx.
c7d455c
to
63b3be9
Compare
|
resolves #103528
This PR is the second part of the
IconButton
migration. In this PR, aIconButton
can be turned into a "toggle button" by usingisSelected
parameter.The boolean
isSelected
is a new parameter inIconButton
class. The button will be non-toggle button by default ifisSelected
is not set; otherwise, the button will show the selection state based on the value ofisSelected
.Pre-launch Checklist
///
).