[Material] ToggleButtons #34599
[Material] ToggleButtons #34599
Conversation
Not robustly handling active/disabled/enabled states
…l layout update logic
shihaohong
added
c: new feature
HansMuller
left a comment
HansMuller
left a comment
There was a problem hiding this comment.
Mostly trivial feedback; this PR is pretty close.
| /// | ||
| /// The list of [children] are laid out in a row. The state of each button | ||
| /// is controlled by [isSelected], which is a list of bools that determine | ||
| /// if a button is in an active, disabled, or selected state. They are both |
There was a problem hiding this comment.
I don't think the isSelected list controls the disabled state (it's elements must be true or false - not null - after all).
| /// match the length of the [children] list. | ||
| /// | ||
| /// ## Customizing toggle buttons | ||
| /// The toggle buttons are designed to be configurable, meaning the actions |
There was a problem hiding this comment.
Maybe too many words for not enough content. I assume that what you're trying to say is that each toggle's behavior is defined by its [onPressed] callback which can update the [isSelected] list however it wants to.
| /// designed. This can be configured using the [onPressed] callback, which | ||
| /// is triggered when a button is pressed. | ||
| /// | ||
| /// Here is an implementation that allows for multiple buttons to be |
| /// ], | ||
| /// onPressed: (int index) { | ||
| /// setState(() { | ||
| /// for (int currentIndex = 0; currentIndex < isSelected.length; currentIndex++) { |
There was a problem hiding this comment.
Might be a photon's weight clearer to call currentIndex buttonIndex.
| /// ``` | ||
| /// | ||
| /// ## ToggleButton Borders | ||
| /// The toggle buttons, by default, have a solid, 1 dp border surrounding itself |
There was a problem hiding this comment.
We usually say "logical pixel" or just "pixel" instead of dp.
| ); | ||
| } else { | ||
| return BorderSide( | ||
| color: disabledBorderColor ?? toggleButtonsTheme.disabledBorderColor ?? theme.disabledColor, |
There was a problem hiding this comment.
Can we derive the disabledColor from the color scheme? Here and below
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| assert( |
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| assert( |
|
|
||
| @override | ||
| double computeMaxIntrinsicHeight(double width) { | ||
| return horizontalBorderSide.width + |
There was a problem hiding this comment.
This expression and the similar ones below can be one liners.
There was a problem hiding this comment.
I was originally thinking that having multiple succinct lines was better than having a massive one-liner, since it might be easier to read/parse it if it were multiple lines.
Primarily driven by style guide suggestions, but let me know what you think!
|
|
||
| /// The radii of the border's corners. | ||
| /// | ||
| /// By default, the border's corners are not rounded. |
There was a problem hiding this comment.
Don't need to mention default values here.
Also refactored code to be more readable
…owing colorSchemes
HansMuller
left a comment
HansMuller
left a comment
There was a problem hiding this comment.
Nice work! This maybe the first new widget and theme to be depend exclusively on the color scheme and to follow the state of the art theme pattern.
|
Thank you, @shihaohong !!! |
* Introduces ToggleButtons widget * Introduces ToggleButtonsTheme inherited widget * Introduces ThemeData.toggleButtonsTheme property
|
Hi @frmatthew, I created a separate issue to track your request: #39216. |
|
Where can I get the full code for this toggle button? |
9 participants
Description
Implemented ToggleButtons widget. It is a horizontal set of toggle buttons, which are usually Text/Icon widgets.
Toggle buttons are designed to be configurable, meaning the actions performed by tapping a toggle button and the desired interface can be designed through defining the onPressed callback. See API doc examples for different suggestions.
Related Issues
Fixes #3959
Related to #35088
Tests
I added the following tests:
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.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Does your PR require Flutter developers to manually update their apps to accommodate your change?