Refactor `ToggleButtons` (remove `RawMaterialButton`) #99493

TahaTesser · 2022-03-03T17:09:28Z

minimal code sample

import 'package:flutter/material.dart';

const List<Widget> icons = <Widget>[
  Icon(Icons.ac_unit),
  Icon(Icons.call),
  Icon(Icons.cake),
];

void main() => runApp(const MyApp());

class MyApp extends StatelessWidget {
  const MyApp({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      debugShowCheckedModeBanner: false,
      title: 'Material App',
      home: ToggleButtonsSample(),
    );
  }
}

class ToggleButtonsSample extends StatelessWidget {
  ToggleButtonsSample({Key? key}) : super(key: key);
  final ValueNotifier<int> _toggleValue = ValueNotifier<int>(0);

  @override
  Widget build(BuildContext context) {
    final ColorScheme colorScheme = Theme.of(context).colorScheme;

    return Scaffold(
      appBar: AppBar(
        title: const Text('ToggleButtons Sample'),
      ),
      body: Center(
        child: ValueListenableBuilder(
          valueListenable: _toggleValue,
          builder: (BuildContext context, int value, Widget? child) {
            return ToggleButtons(
              onPressed: (int newIndex) => _toggleValue.value = newIndex,
              borderRadius: const BorderRadius.all(Radius.circular(8)),
              selectedBorderColor: colorScheme.primary,
              selectedColor: colorScheme.primary,
              isSelected: <bool>[
                _toggleValue.value == 0,
                _toggleValue.value == 1,
                _toggleValue.value == 2,
              ],
              children: icons,
            );
          },
        ),
      ),
    );
  }
}

Description

Here this PR refactors ToggleButtons and use ButtonStyleButton to style ToggleButtons

highlight color not being translated to ElevatedButton (ButtonStyleButton sets highlight color to be transparent).

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.

Piinks

NICE - all of the existing golden file tests pass! 🎉 (There are only two, but still, that's great)

Comparing this to the same refactor you are doing for FAB (#99753)

FAB is not becoming a ButtonStyleButton, but here it looks like ToggleButtons are.
FAB is not going to have a ButtonStyle style parameter like here, instead deferring to its numerated properties and inherited theme to create a ButtonStyle internally. ToggleButtons here allow for style to be set OR individual properties + theming.

I wonder if the FAB approach is cleaner, the style property here on ToggleButtons might not be necessary, but again I will defer to @HansMuller on general design since he wrote the model.

All of this work in FAB & Toggle is really great progress though. Thanks!

HansMuller · 2022-03-11T00:13:24Z

We should make sure that this PR also fixes #97302

HansMuller · 2022-03-11T00:14:49Z

I agree with Kate, this PR shouldn't include the extra step of adding a ButtonStyle style parameter to ToggleButtons; we should leave that for a separate proposal/PR.

TahaTesser · 2022-03-14T16:38:34Z

We should make sure that this PR also fixes #97302

This is now handled by TextButton widget in this refactor, can you please take a look

Mobile

Desktop (Web)

HansMuller · 2022-03-14T18:33:25Z

@TahaTesser - if this PR fixes #97302 it should say so in the PR's description and it should include a regression test (marked as a regression test for #97302 with a comment).

HansMuller · 2022-03-14T18:34:25Z

@TahaTesser - the screenshots in #99493 (comment) imply that the background color for the ToggleButtons isn't clipped to the ToggleButtons' shape?

TahaTesser · 2022-03-14T21:45:46Z

@HansMuller
Thanks so much for your time,

@TahaTesser - the screenshots in #99493 (comment) imply that the background color for the ToggleButtons isn't clipped to the ToggleButtons' shape?

This is just a parent container with color from #97302

ClipRRect still clipping the toggle buttons in this refactor
https://github.com/flutter/flutter/pull/99493/files#diff-153bdbb7a6739dd071bbb7b8230e3884dcb6cedbc21a78ef82c0e6a8166bb220R731

HansMuller · 2022-03-14T21:50:43Z

@TahaTesser - Good!

TahaTesser · 2022-03-14T22:49:06Z

@HansMuller
Added a regression test with a link to the issue

TahaTesser · 2022-03-15T09:57:49Z

@HansMuller
During refactoring noticed, there is no interactive example that showcases different configurations for ToggleButtons

Filed #100124, this will land after this PR (tester finders look for TextButton instead of RawMaterialButton).

HansMuller

This is looking good however I don't think the style parameter belongs in this PR; see #99493 (comment)

TahaTesser · 2022-03-17T15:48:26Z

This is looking good however I don't think the style parameter belongs in this PR; see #99493 (comment)

My apologies I missed removing that parameter, done.

packages/flutter/lib/src/material/toggle_buttons.dart

Piinks

@TahaTesser is this ready for another review? Or is it waiting on additional changes like the FAB one?

TahaTesser · 2022-03-23T10:07:09Z

@TahaTesser is this ready for another review? Or is it waiting on additional changes like the FAB one?

Hey @Piinks!
This needs another review. (my bad I thought I click request button before)

This Diagnosticable issue from FAB one doesn't affect this PR, all tests are passing

packages/flutter/lib/src/material/toggle_buttons.dart

HansMuller · 2022-03-23T16:28:12Z

@TahaTesser - I haven't tested or even compiled the code I've suggested for the _ForegroundColor and _BackgroundColor. If they're difficult let me know and I'll fix them.

TahaTesser · 2022-03-23T21:32:46Z

@TahaTesser - I haven't tested or even compiled the code I've suggested for the _ForegroundColor and _BackgroundColor. If they're difficult let me know and I'll fix them.

This is cleaner, thanks!
I made slight refinements and added these classes including the local ColorScheme variable as suggested and removed the redundant code due to these classes.

…r/flutter#99493)

math1man · 2022-04-07T18:58:19Z

Hey, I think this change is a incorrect. #99493 (comment) does not look like the correct toggle button styling on Mobile.

HansMuller · 2022-04-07T19:18:01Z

@math1man - can you explain what aspect of the component doesn't look correct?

math1man · 2022-04-07T19:18:55Z

It's too tall. The standard height I believe is 32 dp, but it looks like 48 dp.

HansMuller · 2022-04-07T19:36:14Z

@TahaTesser - can you investigate? AFAICT we have not changed the geometry of ToggleButtons, however perhaps we've always had this wrong. I don't believe https://m3.material.io/ covers it (yet).

This reverts commit 0b216f7.

Piinks · 2022-04-07T19:53:45Z

The failure appears related to #97302, which this closed. Rather than allow for a height of 32, it seems like this enforced a height of 48 - IIRC that is the minimum required size for interactive elements. To reproduce this, the ToggleButtons.constraints were set with a min/max height of 32, but did not adhere to that.

…r#99493)" This reverts commit 0b216f7.

TahaTesser · 2022-04-07T20:42:01Z

@HansMuller

I also found this in the Material Design components app, I will look into it and test it in an Android native app.

Preview

Perhaps there should Android equivalent of https://github.com/flutter/platform_tests/tree/master/ios_widget_catalog_compare

TahaTesser · 2022-04-07T20:43:12Z

@Piinks
Thank you for reverting

…" (#101538) This reverts commit 0b216f7.

…" (#101536) This reverts commit 0b216f7.

HansMuller · 2022-04-07T21:56:35Z

@TahaTesser - If a PR is created that relands the ToggleButton refactor, we'll need some additional regression tests to verify that the buttons handle input within 48 vertical without the button's outline occupying 48.

* 'stable' of https://github.com/flutter/flutter: (1203 commits) 'Update Engine revision to ffe7b86a1e5b5cb63c8385ae1adc759e372ee8f4 for stable release 3.0.3' (flutter#106431) [flutter_releases] Removing minor iOS version filter value from ci.yaml (flutter#105059) (flutter#105629) [flutter_releases] Flutter stable 3.0.2 Framework Cherrypicks (flutter#105528) [framework] ensure ink sparkle is disposed (flutter#104569) (flutter#105144) [CP] Fix Flutter doctor crash on Windows caused by bad UTF8 from vswhere.exe (flutter#105153) [tool][web] Use 'constructor' instead of 'public field declarations' in flutter.js (Safari 13) (flutter#105162) [flutter_releases] Cherrypicks for SliverReorderableList and Slider regressions (flutter#105141) 'Update Engine revision to caaafc5604ee9172293eb84a381be6aadd660317 for stable release 3.0.1' (flutter#104213) [flutter_releases] Flutter stable 2.13.0 Framework Cherrypicks (flutter#103375) [flutter_releases] Flutter beta 2.13.0-0.4.pre Framework Cherrypicks (flutter#103101) Enforce cpu explicitly for Mac devicelab test beds (flutter#101871) (flutter#102685) [flutter_releases] Flutter beta 2.13.0-0.3.pre Framework Cherrypicks (flutter#102620) [flutter_releases] Upgrade dwds to 12.1.1 (flutter#101546) [flutter_releases] Flutter beta 2.13.0-0.2.pre Framework Cherrypicks (flutter#102193) [flutter_releases] Flutter beta 2.12.0-4.1.pre Framework Cherrypicks (flutter#101771) [flutter_releases] Cherrypick engine to c341645 (flutter#101608) Revert "Refactor `ToggleButtons` (remove `RawMaterialButton`) (flutter#99493)" (flutter#101538) [Cherrypick] Partial revert of super params in tools (flutter#101436) (flutter#101527) Roll Engine from e7e7ca1 to b48d65e (10 revisions) (flutter#101370) [flutter_tools] port bash script to use sysctl not uname on macOS (flutter#101308) ...

…flutter#99493)" (flutter#101538)" This reverts commit 02d44bb.

flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Mar 3, 2022

TahaTesser requested review from HansMuller and Piinks March 3, 2022 17:44

TahaTesser force-pushed the refactor_toggle_buttons branch from 8058dd0 to 2f79f04 Compare March 4, 2022 10:32

Piinks reviewed Mar 10, 2022

View reviewed changes

TahaTesser force-pushed the refactor_toggle_buttons branch from 74a9652 to 4b1618d Compare March 11, 2022 15:07

TahaTesser force-pushed the refactor_toggle_buttons branch from 9d5b89e to 4284e86 Compare March 14, 2022 22:56

TahaTesser mentioned this pull request Mar 15, 2022

ToggleButtons: Add interactive example #100124

Merged

8 tasks

HansMuller reviewed Mar 16, 2022

View reviewed changes

TahaTesser force-pushed the refactor_toggle_buttons branch from 4284e86 to 21d345d Compare March 17, 2022 15:47

HansMuller reviewed Mar 17, 2022

View reviewed changes

packages/flutter/lib/src/material/toggle_buttons.dart Outdated Show resolved Hide resolved

packages/flutter/lib/src/material/toggle_buttons.dart Outdated Show resolved Hide resolved

packages/flutter/lib/src/material/toggle_buttons.dart Outdated Show resolved Hide resolved

TahaTesser mentioned this pull request Mar 22, 2022

Refactor FAB (Remove RawMaterialButton) #99753

Closed

8 tasks

Piinks reviewed Mar 22, 2022

View reviewed changes

TahaTesser requested a review from HansMuller March 23, 2022 10:07

HansMuller reviewed Mar 23, 2022

View reviewed changes

packages/flutter/lib/src/material/toggle_buttons.dart Outdated Show resolved Hide resolved

packages/flutter/lib/src/material/toggle_buttons.dart Outdated Show resolved Hide resolved

TahaTesser force-pushed the refactor_toggle_buttons branch from 213dfc0 to 9db5792 Compare March 23, 2022 20:40

engine-flutter-autoroll mentioned this pull request Apr 5, 2022

Roll Flutter from 66ed64be4fd8 to bc0ec85717de (65 revisions) flutter/plugins#5170

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Apr 5, 2022

0b216f7 Refactor ToggleButtons (remove RawMaterialButton) (flutte…

01c997f

…r/flutter#99493)

engine-flutter-autoroll mentioned this pull request Apr 5, 2022

Roll Flutter from 66ed64be4fd8 to 785513234141 (66 revisions) flutter/plugins#5171

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Apr 5, 2022

0b216f7 Refactor ToggleButtons (remove RawMaterialButton) (flutte…

2b9f995

…r/flutter#99493)

engine-flutter-autoroll mentioned this pull request Apr 5, 2022

Roll Flutter from 66ed64be4fd8 to 785513234141 (66 revisions) flutter/plugins#5173

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Apr 5, 2022

0b216f7 Refactor ToggleButtons (remove RawMaterialButton) (flutte…

fff71a9

…r/flutter#99493)

engine-flutter-autoroll mentioned this pull request Apr 5, 2022

Roll Flutter from 66ed64be4fd8 to 647044c969af (67 revisions) flutter/plugins#5175

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Apr 5, 2022

0b216f7 Refactor ToggleButtons (remove RawMaterialButton) (flutte…

82f9cb9

…r/flutter#99493)

engine-flutter-autoroll mentioned this pull request Apr 5, 2022

Roll Flutter from 66ed64be4fd8 to d6cc67d52939 (68 revisions) flutter/plugins#5176

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Apr 5, 2022

0b216f7 Refactor ToggleButtons (remove RawMaterialButton) (flutte…

c317616

…r/flutter#99493)

engine-flutter-autoroll mentioned this pull request Apr 5, 2022

Roll Flutter from 66ed64be4fd8 to 35b18ba2e902 (70 revisions) flutter/plugins#5177

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Apr 5, 2022

0b216f7 Refactor ToggleButtons (remove RawMaterialButton) (flutte…

0e8f7e1

…r/flutter#99493)

engine-flutter-autoroll mentioned this pull request Apr 5, 2022

Roll Flutter from 5c6918933e15 to 5788f5ef07c2 (61 revisions) flutter/plugins#5179

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Apr 5, 2022

0b216f7 Refactor ToggleButtons (remove RawMaterialButton) (flutte…

01652f6

…r/flutter#99493)

Piinks added a commit that referenced this pull request Apr 7, 2022

Revert "Refactor ToggleButtons (remove RawMaterialButton) (#99493)"

082e469

This reverts commit 0b216f7.

Piinks mentioned this pull request Apr 7, 2022

Revert "Refactor ToggleButtons (remove RawMaterialButton)" #101536

Merged

CaseyHillers pushed a commit to CaseyHillers/flutter that referenced this pull request Apr 7, 2022

Revert "Refactor ToggleButtons (remove RawMaterialButton) (flutte…

8df8656

…r#99493)" This reverts commit 0b216f7.

CaseyHillers pushed a commit that referenced this pull request Apr 7, 2022

Revert "Refactor ToggleButtons (remove RawMaterialButton) (#99493)…

02d44bb

…" (#101538) This reverts commit 0b216f7.

Piinks added a commit that referenced this pull request Apr 7, 2022

Revert "Refactor ToggleButtons (remove RawMaterialButton) (#99493)…

f5be639

…" (#101536) This reverts commit 0b216f7.

TahaTesser mentioned this pull request Apr 12, 2022

[reland] Refactor ToggleButtons (remove RawMaterialButton) #101760

Merged

8 tasks

pieter-scholtz added a commit to pieter-scholtz/flutter that referenced this pull request Jul 7, 2022

Revert "Revert "Refactor ToggleButtons (remove RawMaterialButton) (…

a022d79

…flutter#99493)" (flutter#101538)" This reverts commit 02d44bb.

Refactor ToggleButtons (remove RawMaterialButton) #99493

Refactor ToggleButtons (remove RawMaterialButton) #99493

Uh oh!

Conversation

TahaTesser commented Mar 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pre-launch Checklist

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

HansMuller commented Mar 11, 2022

Uh oh!

HansMuller commented Mar 11, 2022

Uh oh!

TahaTesser commented Mar 14, 2022

Mobile

Desktop (Web)

Uh oh!

HansMuller commented Mar 14, 2022

Uh oh!

HansMuller commented Mar 14, 2022

Uh oh!

TahaTesser commented Mar 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HansMuller commented Mar 14, 2022

Uh oh!

TahaTesser commented Mar 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TahaTesser commented Mar 15, 2022

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

TahaTesser commented Mar 17, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

TahaTesser commented Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HansMuller commented Mar 23, 2022

Uh oh!

TahaTesser commented Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

math1man commented Apr 7, 2022

Uh oh!

HansMuller commented Apr 7, 2022

Uh oh!

math1man commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HansMuller commented Apr 7, 2022

Uh oh!

Piinks commented Apr 7, 2022

Uh oh!

TahaTesser commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TahaTesser commented Apr 7, 2022

Uh oh!

HansMuller commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Refactor `ToggleButtons` (remove `RawMaterialButton`) #99493

Refactor `ToggleButtons` (remove `RawMaterialButton`) #99493

TahaTesser commented Mar 3, 2022 •

edited

Loading

TahaTesser commented Mar 14, 2022 •

edited

Loading

TahaTesser commented Mar 14, 2022 •

edited

Loading

TahaTesser commented Mar 23, 2022 •

edited

Loading

TahaTesser commented Mar 23, 2022 •

edited

Loading

math1man commented Apr 7, 2022 •

edited

Loading

TahaTesser commented Apr 7, 2022 •

edited

Loading

HansMuller commented Apr 7, 2022 •

edited

Loading