Update margin between label and icon in Tab to better reflect Material specs #140698

davidmartos96 · 2023-12-28T12:26:26Z

This PR improves the distance between the label and the icon in the Tab widget.
I updated the margin to 2 pixels, taken from the Figma design page for Material 3. On Material 2 I left the default value of 10 pixels.

Related to #128696 (In particular, the distance between label and icon)

Here are some screenshots for comparison. I looked a bit into the other mentioned issue of the tab height not following the M3 spec. Flutter uses 72 and the spec uses 64. But because Tab is a PreferredSizeWidget, I don't think there is an easy way to provide a different size depending on ThemeData.useMaterial3, because there is no BuildContext available.
I provide a sample image for the 64 height as well for context on the linked issue, even though it's not part of the PR changes.

The screenshots are taken side by side with the image at: https://m3.material.io/components/tabs/guidelines

Original

New (tab height = 72, Flutter default for 8 years)

New (tab height = 64, M3 spec)

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 · 2023-12-28T12:26:29Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

flutter-dashboard · 2023-12-28T13:52:13Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #140698 at sha 2391d16

Piinks

Hey @davidmartos96 thanks for contributing.

Flutter uses 72 and the spec uses 64. But because Tab is a PreferredSizeWidget, I don't think there is an easy way to provide a different size depending on ThemeData.useMaterial3, because there is no BuildContext available.

This is exactly right 💯 , and why is has not been updated before now. Since useMaterial3 is true by default now, we can likely update the preferred size to match the spec now. This would likely be best as a separate change though, it will likely take some time to land since there will be a lot of affected customers.

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

flutter-dashboard · 2024-01-09T15:18:41Z

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #140698 at sha 4d4b53b

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

packages/flutter/test/material/tabs_test.dart

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

davidmartos96 · 2024-01-09T16:21:06Z

@TahaTesser Kind of on-topic with m2 and m3. I'm not sure if you know the answer.
There are a few tests in tabs_test.dart written like this:

  testWidgets('Tab sizing - icon and text', (WidgetTester tester) async {
    final ThemeData theme = ThemeData(fontFamily: 'FlutterTest');
    final bool material3 = theme.useMaterial3;
    await tester.pumpWidget(
      MaterialApp(theme: theme, home: const Center(child: Material(child: Tab(icon: SizedBox(width: 10.0, height: 10.0), text: 'x')))),
    );
    expect(tester.renderObject<RenderParagraph>(find.byType(RichText)).text.style!.fontFamily, 'FlutterTest');
    expect(
      tester.getSize(find.byType(Tab)),
      material3 ? const Size(14.25, 72.0) : const Size(14.0, 72.0));
  });

Material 3 would always be true right now, so there are "hanging" expectations. Is this part of #139076 and just missing some refactoring?

flutter-dashboard · 2024-01-10T10:32:32Z

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #140698 at sha e156d85

TahaTesser · 2024-01-11T09:27:27Z

@TahaTesser Kind of on-topic with m2 and m3. I'm not sure if you know the answer. There are a few tests in tabs_test.dart written like this:

Yep, some of the old tests isn't following format rules. I recommend following the correct format for new changes even if you duplicate old tests and only make changes related to the fix itself, not update unrelated tests.

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

packages/flutter/test/material/tabs_test.dart

Co-authored-by: Taha Tesser <[email protected]>

flutter-dashboard · 2024-01-11T10:33:19Z

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #140698 at sha 88d8577

packages/flutter/test/material/tabs_test.dart

flutter-dashboard · 2024-01-11T16:17:20Z

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #140698 at sha a889db9

TahaTesser

This is almost there! Thanks for working on this.

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

dev/tools/gen_defaults/lib/tabs_template.dart

packages/flutter/test/material/tabs_test.dart

flutter-dashboard · 2024-01-17T09:28:25Z

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #140698 at sha 411694d

TahaTesser

LGTM!

Piinks

Thank you!

… Material specs (flutter/flutter#140698)

Manual roll Flutter from f77f824 to 684247a (39 revisions) Manual roll requested by [email protected] flutter/flutter@f77f824...684247a 2024-01-19 [email protected] Use Integer instead of int in map in flutter.groovy (flutter/flutter#141895) 2024-01-19 [email protected] Roll Flutter Engine from c953c83112ba to f2b441a26416 (4 revisions) (flutter/flutter#141894) 2024-01-19 [email protected] Native assets: package in framework on iOS and MacOS (flutter/flutter#140907) 2024-01-19 [email protected] Revert "Make tests more resilient to Skia gold failures and refactor flutter_goldens for extensive technical debt removal (#140101)" (flutter/flutter#141814) 2024-01-19 [email protected] Roll Flutter Engine from 538975f2511b to c953c83112ba (3 revisions) (flutter/flutter#141886) 2024-01-19 [email protected] Add `showDragHandle` to `showBottomSheet` (flutter/flutter#141754) 2024-01-19 [email protected] Roll Flutter Engine from 9a6c64de8a46 to 538975f2511b (8 revisions) (flutter/flutter#141881) 2024-01-19 [email protected] Make pumpWidget's arguments named (flutter/flutter#141728) 2024-01-19 [email protected] Tiny fix inaccurate documentations about bindings (flutter/flutter#140282) 2024-01-19 [email protected] Roll engine to 9a6c64de8a4694cef59a338cd33ac1a9e7d23d9d (flutter/flutter#141870) 2024-01-19 [email protected] Roll Packages from 83c2c4d to 129e08c (13 revisions) (flutter/flutter#141865) 2024-01-19 [email protected] Add mac_x64_ios configuration. (flutter/flutter#141828) 2024-01-19 [email protected] Roll Flutter Engine from 90be25d8aac3 to d1afda52d254 (1 revision) (flutter/flutter#141825) 2024-01-19 [email protected] Move the requestKeyboard up to the widgets layer (flutter/flutter#141655) 2024-01-19 [email protected] Roll Flutter Engine from dde3ebf6551a to 90be25d8aac3 (1 revision) (flutter/flutter#141817) 2024-01-18 [email protected] enable more tests in web mode (flutter/flutter#141791) 2024-01-18 [email protected] Roll Flutter Engine from 9dded186bcff to dde3ebf6551a (2 revisions) (flutter/flutter#141811) 2024-01-18 [email protected] Update margin between label and icon in Tab to better reflect Material specs (flutter/flutter#140698) 2024-01-18 [email protected] Roll Flutter Engine from 3106e08e1219 to 9dded186bcff (2 revisions) (flutter/flutter#141807) 2024-01-18 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Enable native compilation for windows-arm64 " (flutter/flutter#141809) 2024-01-18 [email protected] Run framework_tests_misc in arm64 and x64. (flutter/flutter#141797) 2024-01-18 [email protected] Roll Flutter Engine from f4a4f046b173 to 3106e08e1219 (1 revision) (flutter/flutter#141802) 2024-01-18 49699333+dependabot[bot]@users.noreply.github.com Bump actions/upload-artifact from 4.1.0 to 4.2.0 (flutter/flutter#141803) 2024-01-18 [email protected] Update labeler.yml (flutter/flutter#141697) 2024-01-18 [email protected] Roll Flutter Engine from 75400c49fa0b to f4a4f046b173 (2 revisions) (flutter/flutter#141800) 2024-01-18 [email protected] Reland "Remove hack from PageView." (flutter/flutter#141533) 2024-01-18 [email protected] ScaleGestureRecognizer pointerCount=2 for trackpad gestures (flutter/flutter#140745) 2024-01-18 [email protected] Roll Flutter Engine from de68e7612948 to 75400c49fa0b (2 revisions) (flutter/flutter#141796) 2024-01-18 [email protected] Run `flutter_gallery_ios__start_up` test on Mac-14 in staging (flutter/flutter#141795) 2024-01-18 [email protected] Roll Flutter Engine from d80fe1cb5854 to de68e7612948 (1 revision) (flutter/flutter#141789) 2024-01-18 [email protected] Enable native compilation for windows-arm64 (flutter/flutter#137618) 2024-01-18 [email protected] [github actions] Fix token issue on actions/checkout package (flutter/flutter#141652) 2024-01-18 [email protected] Roll Flutter Engine from b75d6d80d813 to d80fe1cb5854 (2 revisions) (flutter/flutter#141785) 2024-01-18 [email protected] Revert "Native assets: roll deps" (flutter/flutter#141748) 2024-01-18 [email protected] Deprecate M2 curves (flutter/flutter#134417) 2024-01-18 [email protected] Fix: TextField can inherit `errorStyle` from `InputDecorationTheme`. (flutter/flutter#141227) 2024-01-18 [email protected] Add check for Bank of Brazil security module to Windows Flutter Doctor validators (flutter/flutter#141135) 2024-01-18 [email protected] Fix gradle lints No semantic change should be present. (flutter/flutter#141692) 2024-01-18 [email protected] Roll Packages from 1a2b780 to 83c2c4d (5 revisions) (flutter/flutter#141778) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human ...

Update iconMargin in Tab to better reflect Material specs

88b89f2

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Dec 28, 2023

davidmartos96 added 2 commits December 28, 2023 13:45

add test

d43c722

remove trailing space

2391d16

davidmartos96 force-pushed the fix_tabs branch from 436c262 to 2391d16 Compare December 28, 2023 13:20

flutter-dashboard bot added the will affect goldens Changes to golden files label Dec 28, 2023

HansMuller requested a review from Piinks January 5, 2024 22:59

Piinks reviewed Jan 8, 2024

View reviewed changes

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

TahaTesser requested changes Jan 9, 2024

View reviewed changes

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

davidmartos96 added 2 commits January 9, 2024 15:25

Merge remote-tracking branch 'origin/main' into fix_tabs

6a3049b

add margin to material defaults as a static value

4d4b53b

TahaTesser requested changes Jan 9, 2024

View reviewed changes

Address code review

f3c84f7

Empty-Commit

e156d85

TahaTesser requested changes Jan 11, 2024

View reviewed changes

Apply suggestions from code review

88d8577

Co-authored-by: Taha Tesser <[email protected]>

TahaTesser requested changes Jan 11, 2024

View reviewed changes

packages/flutter/test/material/tabs_test.dart Outdated Show resolved Hide resolved

formatting

a889db9

davidmartos96 requested a review from TahaTesser January 11, 2024 15:46

TahaTesser requested changes Jan 16, 2024

View reviewed changes

simplify tests

1347eb1

davidmartos96 added 2 commits January 16, 2024 15:56

update expectations

ef5518c

Merge branch 'master' into fix_tabs

411694d

TahaTesser approved these changes Jan 17, 2024

View reviewed changes

TahaTesser requested a review from Piinks January 17, 2024 10:44

Piinks approved these changes Jan 18, 2024

View reviewed changes

Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 18, 2024

auto-submit bot merged commit 197cd4d into flutter:master Jan 18, 2024

XilaiZhang added cp: beta cherry pick this pull request to beta release candidate branch and removed cp: beta cherry pick this pull request to beta release candidate branch labels Jan 18, 2024

engine-flutter-autoroll mentioned this pull request Jan 19, 2024

Roll Flutter from f77f82435a97 to c95bb6fbb33a (28 revisions) flutter/packages#5939

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 19, 2024

197cd4d Update margin between label and icon in Tab to better reflect…

ee202c4

… Material specs (flutter/flutter#140698)

engine-flutter-autoroll mentioned this pull request Jan 19, 2024

Roll Flutter from f77f82435a97 to 0487931ac208 (29 revisions) flutter/packages#5940

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 19, 2024

197cd4d Update margin between label and icon in Tab to better reflect…

59aa911

… Material specs (flutter/flutter#140698)

engine-flutter-autoroll mentioned this pull request Jan 19, 2024

Roll Flutter from f77f82435a97 to d4707d12d5b6 (30 revisions) flutter/packages#5942

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 19, 2024

197cd4d Update margin between label and icon in Tab to better reflect…

19630e2

… Material specs (flutter/flutter#140698)

engine-flutter-autoroll mentioned this pull request Jan 19, 2024

Roll Flutter from f77f82435a97 to 9e024fdf3122 (31 revisions) flutter/packages#5943

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 19, 2024

197cd4d Update margin between label and icon in Tab to better reflect…

f0f64a6

… Material specs (flutter/flutter#140698)

engine-flutter-autoroll mentioned this pull request Jan 19, 2024

Roll Flutter from f77f82435a97 to cc544169be01 (32 revisions) flutter/packages#5945

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 19, 2024

197cd4d Update margin between label and icon in Tab to better reflect…

faa437f

… Material specs (flutter/flutter#140698)

engine-flutter-autoroll mentioned this pull request Jan 19, 2024

Roll Flutter from f77f82435a97 to 3563372f0670 (33 revisions) flutter/packages#5946

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 19, 2024

197cd4d Update margin between label and icon in Tab to better reflect…

07cdbfe

… Material specs (flutter/flutter#140698)

HansMuller mentioned this pull request Jan 19, 2024

Flutter Framework Team: Notable Commits #121415

Closed

engine-flutter-autoroll mentioned this pull request Jan 19, 2024

Manual roll Flutter from f77f82435a97 to 684247a3c78a (39 revisions) flutter/packages#5948

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 19, 2024

197cd4d Update margin between label and icon in Tab to better reflect…

2c6e800

… Material specs (flutter/flutter#140698)

davidmartos96 deleted the fix_tabs branch January 20, 2024 00:12

davidmartos96 mentioned this pull request Jan 22, 2024

M3 tabs spec issues #128696

Closed

3 tasks

Update margin between label and icon in Tab to better reflect Material specs #140698

Update margin between label and icon in Tab to better reflect Material specs #140698

Uh oh!

Conversation

davidmartos96 commented Dec 28, 2023 • edited by TahaTesser Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Original

New (tab height = 72, Flutter default for 8 years)

New (tab height = 64, M3 spec)

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Dec 28, 2023

Uh oh!

flutter-dashboard bot commented Dec 28, 2023

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

flutter-dashboard bot commented Jan 9, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

davidmartos96 commented Jan 9, 2024

Uh oh!

flutter-dashboard bot commented Jan 10, 2024

Uh oh!

TahaTesser commented Jan 11, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

flutter-dashboard bot commented Jan 11, 2024

Uh oh!

Uh oh!

flutter-dashboard bot commented Jan 11, 2024

Uh oh!

TahaTesser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

flutter-dashboard bot commented Jan 17, 2024

Uh oh!

TahaTesser left a comment

Choose a reason for hiding this comment

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

davidmartos96 commented Dec 28, 2023 •

edited by TahaTesser

Loading