-
Notifications
You must be signed in to change notification settings - Fork 29.7k
[reland] Refactor ToggleButtons (remove RawMaterialButton) #101760
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
[reland] Refactor ToggleButtons (remove RawMaterialButton) #101760
Conversation
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 new tests checks for button size and semantic node size
button's = 32.0
semantic node's height = 48.0
cc: @HansMuller
|
Nice! Looks great! |
|
I'm afraid I couldn't quite parse the rationale for |
|
Here in the code, toggle buttons are stretched to take all the available space to match the tallest widget flutter/packages/flutter/lib/src/material/toggle_buttons.dart Lines 701 to 716 in 5fb74db
For this test flutter/packages/flutter/test/material/toggle_buttons_test.dart Lines 1177 to 1188 in 5fb74db
This makes toggle buttons take all the available space, and stretch and input paddings are not possible as a result. Here on the left, buttons with On the right, all buttons are stretched to match the tallest widget with various size constraints. Code from
minimal code sampleimport 'package:flutter/material.dart';
class Home extends StatefulWidget {
const Home({Key? key}) : super(key: key);
@override
_HomeState createState() => _HomeState();
}
class _HomeState extends State<Home> {
int selectedTabIndex = 0;
@override
Widget build(BuildContext context) {
final ColorScheme colorScheme = Theme.of(context).colorScheme;
return Scaffold(
body: Center(
child: Container(
color: Colors.green.withOpacity(0.15),
child: ToggleButtons(
direction: Axis.horizontal,
tapTargetSize: MaterialTapTargetSize.padded,
isSelected: [
selectedTabIndex == 0,
selectedTabIndex == 1,
selectedTabIndex == 2,
],
onPressed: (int index) {
setState(() {
selectedTabIndex = index;
});
},
borderRadius: const BorderRadius.all(Radius.circular(8)),
selectedBorderColor: colorScheme.primary,
selectedColor: colorScheme.primary,
// expandChildren: true,
children: <Widget>[
Container(
constraints: const BoxConstraints.tightFor(height: 100.0),
),
Container(
constraints: const BoxConstraints.tightFor(
height: 400.0), // tallest widget
),
Container(
constraints: const BoxConstraints.tightFor(height: 200.0),
),
],
),
),
),
);
}
}
void main() {
runApp(const MaterialApp(home: Home()));
}We need the buttons to take less than
We need buttons not to stretch and leave room for padding when height but as you can see on the right, all buttons are constrained when test expect the buttons to be stretched to match the tallest widget height, hence I added This might not be the best solution, if you have a better idea, please let me know, appreciate it |
|
I'm testing this out now @TahaTesser, thanks for your patience. It looks like there are merge conflicts currently though, just FYI. :) |
f309434 to
5938b96
Compare
Piinks
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.
I think that should work, please take a look and let me know what you think. And as always, thank you!
|
I also made another minor change, since |
|
Cool! I will take another look and run it against the internal test that caused it to be reverted just to be sure. |
|
It looks like the previous test failure is fixed now internally, but we introduced some new ones here. I'll update once I know what broke these other tests. |
|
Still working on resolving the test failures. I am hoping to have them sorted today, a customer has informed us that #97302 is a potential release blocker and this will fix it. |
Piinks
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.
These two adjustments should have this sorted. Can you apply these updates and I'll just double check once more? I think the Google testing shard is fixed now too if you want to pull updates from master. Thanks!
e260c8e to
0a6433f
Compare
|
When you have a chance @TahaTesser can you update the before/after image in your OP? I think the mix up with x and y is what caused the unexpected image this morning. Thank you! |
Restored :) |
|
Update: I am just awaiting a few changes landing in google and then this can land. There were a few customer tests that were looking up the underlying RawMaterialButton. |
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.
@TahaTesser this is still off, this additional Semantics widget should not be here. Excluding the semantics of the child leaves it blank without any context.
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.
See comment here: #101760 (comment)
195d0e7 to
460448a
Compare
509f85b to
460448a
Compare
Piinks
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.





reland #99493
fixes #99085
fixes #97302
Individual buttons are expected to take up all the available space but that restrict providing
_inputPaddingto individual buttons, children can't have smaller size while keeping padding (this requires individual buttons to be wrapped withCenter) so individual buttons only take the space they need and leave room for input padding.But buttons are needed to take all available when you have children of different sizes then all button sizes should match the tallest widget hence adding the
expandChildrenproperty. This property will allow children to expand and take the full space.minimal code sample
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.