Skip to content

Conversation

@TahaTesser
Copy link
Member

@TahaTesser TahaTesser commented Apr 12, 2022

reland #99493

fixes #99085
fixes #97302

Before After

Individual buttons are expected to take up all the available space but that restrict providing _inputPadding to individual buttons, children can't have smaller size while keeping padding (this requires individual buttons to be wrapped with Center) 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 expandChildren property. This property will allow children to expand and take the full space.

minimal code sample
import '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,
            ],
            onPressed: (int index) {
              setState(() {
                selectedTabIndex = index;
              });
            },
            borderRadius: const BorderRadius.all(Radius.circular(8)),
            selectedBorderColor: colorScheme.primary,
            selectedColor: colorScheme.primary,
            constraints: const BoxConstraints.tightFor(
              width: 86,
              height: 32,
            ),
            // expandedChildren: true,
            children: const <Widget>[
              Text('Locks'),
              Text('Unlocks'),
            ],
          ),
        ),
      ),
    );
  }
}

void main() {
  runApp(const MaterialApp(home: Home()));
}

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. labels Apr 12, 2022
@TahaTesser TahaTesser changed the title Refactor ToggleButtons (remove RawMaterialButton) [reland] Refactor ToggleButtons (remove RawMaterialButton) Apr 12, 2022
@TahaTesser
Copy link
Member Author

TahaTesser commented Apr 12, 2022

cc: @math1man

As per your comment, I added padding + semantics to individual buttons, screenshot in the description.

@TahaTesser TahaTesser requested review from HansMuller and Piinks April 12, 2022 17:53
Copy link
Member Author

@TahaTesser TahaTesser Apr 12, 2022

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

@math1man
Copy link
Contributor

Nice! Looks great!

@HansMuller
Copy link
Contributor

I'm afraid I couldn't quite parse the rationale for expandChildren. What's is the scenario where expandChildren is neccessary?

@TahaTesser
Copy link
Member Author

TahaTesser commented Apr 13, 2022

@HansMuller

Here in the code, toggle buttons are stretched to take all the available space to match the tallest widget

final Widget result = direction == Axis.horizontal
? IntrinsicHeight(
child: Row(
mainAxisSize: MainAxisSize.min,
crossAxisAlignment: CrossAxisAlignment.stretch,
children: buttons,
),
)
: IntrinsicWidth(
child: Column(
mainAxisSize: MainAxisSize.min,
crossAxisAlignment: CrossAxisAlignment.stretch,
verticalDirection: verticalDirection,
children: buttons,
),
);

For this test

testWidgets('Height of segmented control is determined by tallest widget', (WidgetTester tester) async {
final List<Widget> 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),
),
];

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 constraints: const BoxConstraints.tightFor(width: 86, height: 32) are stretch to take 48.0 height.

On the right, all buttons are stretched to match the tallest widget with various size constraints. Code from Height of segmented control is determined by the tallest widget test.

Buttons with 32.0 height constraint (stretched to 48.0) Buttons with various height constraints
minimal code sample
import '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 48.0 as per specs (existing design), I added the Center widget so buttons are not stretched and they are allowed to have the actual size they have constrained to.

Buttons with 32.0 Buttons with various height constraints

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 expandChildren which allows buttons to expand when needed. expandChildren basically adds Center widget to buttons so they are not stretched.

This might not be the best solution, if you have a better idea, please let me know, appreciate it

@Piinks
Copy link
Contributor

Piinks commented Apr 19, 2022

I'm testing this out now @TahaTesser, thanks for your patience. It looks like there are merge conflicts currently though, just FYI. :)

@TahaTesser TahaTesser force-pushed the reland_toggle_buttons_factor branch 2 times, most recently from f309434 to 5938b96 Compare April 19, 2022 19:56
Copy link
Contributor

@Piinks Piinks left a 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!

@TahaTesser
Copy link
Member Author

TahaTesser commented Apr 19, 2022

I also made another minor change, since result widget is not wrapped in input padding, We don't need to return it from the if-else statement every time, instead, I am returning the default configuration if the axis direction is the default.

hhttps://github.com/flutter/flutter/pull/101760/files#diff-153bdbb7a6739dd071bbb7b8230e3884dcb6cedbc21a78ef82c0e6a8166bb220R795-R811

@Piinks
Copy link
Contributor

Piinks commented Apr 19, 2022

Cool! I will take another look and run it against the internal test that caused it to be reverted just to be sure.

@Piinks
Copy link
Contributor

Piinks commented Apr 20, 2022

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.

@Piinks
Copy link
Contributor

Piinks commented Apr 21, 2022

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
Piinks previously requested changes Apr 21, 2022
Copy link
Contributor

@Piinks Piinks left a 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!

@TahaTesser TahaTesser force-pushed the reland_toggle_buttons_factor branch from e260c8e to 0a6433f Compare April 22, 2022 09:57
@TahaTesser TahaTesser requested a review from Piinks April 22, 2022 10:53
@Piinks
Copy link
Contributor

Piinks commented Apr 22, 2022

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!

@TahaTesser
Copy link
Member Author

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 :)

@TahaTesser TahaTesser requested a review from Piinks April 22, 2022 13:23
@Piinks
Copy link
Contributor

Piinks commented Apr 25, 2022

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.

Comment on lines 787 to 789
Copy link
Contributor

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.

Copy link
Contributor

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)

@TahaTesser TahaTesser force-pushed the reland_toggle_buttons_factor branch from 195d0e7 to 460448a Compare April 26, 2022 07:45
@TahaTesser TahaTesser requested a review from Piinks April 26, 2022 08:49
@TahaTesser TahaTesser force-pushed the reland_toggle_buttons_factor branch from 509f85b to 460448a Compare April 26, 2022 10:14
@Piinks Piinks dismissed their stale review April 27, 2022 18:43

Completed

Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

Flutter_LGTM

@fluttergithubbot fluttergithubbot merged commit 0c3c38d into flutter:master Apr 27, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 27, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 27, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 27, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 27, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 28, 2022
@TahaTesser TahaTesser deleted the reland_toggle_buttons_factor branch April 28, 2022 06:56
egramond pushed a commit to egramond/flutter that referenced this pull request May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor ToggleButtons ToggleButtons with MaterialTapTargetSize.padded and height constrained to 32

5 participants