Skip to content
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

Add ability to override NavigationDestination.label padding for NavigationBar #158260

Merged

Conversation

TahaTesser
Copy link
Member

@TahaTesser TahaTesser commented Nov 6, 2024

Fixes Long NavigationBar tab titles can't be padded from the sides of the screen

Code sample

expand to view the code sample
import 'package:flutter/material.dart';

void main() {
  runApp(const MyApp());
}

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      debugShowCheckedModeBanner: false,
      theme: ThemeData(
          navigationBarTheme: const NavigationBarThemeData(
        labelTextStyle:
            WidgetStatePropertyAll(TextStyle(overflow: TextOverflow.ellipsis)),
        labelPadding: EdgeInsets.symmetric(horizontal: 16, vertical: 4),
      )),
      home: Scaffold(
        body: Center(
          child: Text(
            'Custom NavigationBar label padding',
            style: Theme.of(context).textTheme.titleMedium,
          ),
        ),
        bottomNavigationBar: NavigationBar(
          destinations: const [
            NavigationDestination(
              icon: Icon(Icons.favorite_rounded),
              label: 'Long Label Text',
            ),
            NavigationDestination(
              // icon: SizedBox.shrink(),
              icon: Icon(Icons.favorite_rounded),
              label: 'Long Label Text',
            ),
            NavigationDestination(
              icon: Icon(Icons.favorite_rounded),
              label: 'Long Label Text',
            ),
          ],
        ),
      ),
    );
  }
}

Default NavigationDestination.label padding with long label

Screenshot 2024-11-06 at 14 30 52

Custom NavigationDestination.label padding with long label

Screenshot 2024-11-06 at 14 32 02

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Nov 6, 2024
@TahaTesser TahaTesser requested a review from QuncCccccc November 6, 2024 14:36
Copy link
Contributor

@QuncCccccc QuncCccccc left a comment

Choose a reason for hiding this comment

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

The change LGTM.

Just have one question, what's the expected behavior after we add paddings for each label? If cutting the text (like the screenshot in the PR description) is expected, then it's fine. But if the expected look is to make the text become 2 lines, then it may cause some alignment issue on the icons because destinations are placed in center of the bar.

Screenshot 2024-11-06 at 12 21 20 PM

@TahaTesser
Copy link
Member Author

what's the expected behavior after we add paddings for each label? If cutting the text (like the screenshot in the PR description) is expected, then it's fine.

It is expected to overflow with ellipses on Android when providing long label.

image

But it's not a default feature in Flutter. The user should provide custom overflow when using padding.

      theme: ThemeData(
          navigationBarTheme: const NavigationBarThemeData(
        labelTextStyle:
            WidgetStatePropertyAll(TextStyle(overflow: TextOverflow.ellipsis)),
        labelPadding: EdgeInsets.symmetric(horizontal: 16, vertical: 4),
      )),

Since the default label text style doesn't set a overflow behavior

@override MaterialStateProperty<TextStyle?>? get labelTextStyle {
return MaterialStateProperty.resolveWith((Set<MaterialState> states) {
final TextStyle style = _textTheme.labelMedium!;
return style.apply(
color: states.contains(MaterialState.disabled)
? _colors.onSurfaceVariant.withOpacity(0.38)
: states.contains(MaterialState.selected)
? _colors.onSurface
: _colors.onSurfaceVariant
);
});
}

@TahaTesser TahaTesser requested a review from QuncCccccc November 7, 2024 08:47
Copy link
Contributor

@QuncCccccc QuncCccccc left a comment

Choose a reason for hiding this comment

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

I see! LGTM! Thanks for adding this feature and helping to improve file format!

But it's not a default feature in Flutter. The user should provide custom overflow when using padding.

I think it's okay since changing the default feature may also cause breaking change haha.

@TahaTesser TahaTesser added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 7, 2024
@TahaTesser TahaTesser force-pushed the navigation_destination_labelPadding branch from 1ac7d78 to a0f87d4 Compare November 7, 2024 21:33
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 7, 2024
Copy link
Contributor

auto-submit bot commented Nov 7, 2024

auto label is removed for flutter/flutter/158260, due to - The status or check suite Mac plugin_dependencies_test has failed. Please fix the issues identified (or deflake) before re-applying this label.

@TahaTesser TahaTesser added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 7, 2024
Copy link
Contributor

auto-submit bot commented Nov 8, 2024

auto label is removed for flutter/flutter/158260, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 8, 2024
@TahaTesser TahaTesser added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 8, 2024
@auto-submit auto-submit bot merged commit 7abb083 into flutter:master Nov 8, 2024
140 checks passed
@TahaTesser TahaTesser deleted the navigation_destination_labelPadding branch November 8, 2024 14:24
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 8, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 8, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Nov 11, 2024
flutter/flutter@73546b3...c8510f2

2024-11-08 [email protected] Roll Flutter Engine from 44d788f4651b to a08bd5a07c2a (3 revisions) (flutter/flutter#158375)
2024-11-08 [email protected] Add ability to override `NavigationDestination.label` padding for `NavigationBar` (flutter/flutter#158260)
2024-11-08 [email protected] Add flutter/package code generation instructions (flutter/flutter#158326)
2024-11-08 [email protected] Roll Flutter Engine from 8e19915c19fc to 44d788f4651b (3 revisions) (flutter/flutter#158362)
2024-11-08 [email protected] Roll Flutter Engine from bcb281cde579 to 8e19915c19fc (4 revisions) (flutter/flutter#158354)
2024-11-07 [email protected] Make `_SelectableRegionSelectionContainerDelegate` public (flutter/flutter#147080)
2024-11-07 [email protected] Manual roll Flutter Engine from 371c86fb6b49 to bcb281cde579 (flutter/flutter#158346)
2024-11-07 [email protected] Add clarification on review timelines in PR template (flutter/flutter#158345)
2024-11-07 [email protected] Increase Java heap limit to 8GB for plugin integration tests using deferred components (flutter/flutter#158330)
2024-11-07 [email protected] Roll pub packages (flutter/flutter#158337)
2024-11-07 [email protected] Roll Flutter Engine from ac50b20ae5c9 to 371c86fb6b49 (5 revisions) (flutter/flutter#158336)
2024-11-07 [email protected] Fix a breakage caused by the test being unskipped. (flutter/flutter#158335)
2024-11-07 [email protected] Roll Flutter Engine from 8a963cfc134c to ac50b20ae5c9 (1 revision) (flutter/flutter#158308)
2024-11-07 [email protected] `Plugin.isDevDependency` if exclusively in `dev_dependencies` (flutter/flutter#157462)
2024-11-07 [email protected] Add recently imported packages to issue template (flutter/flutter#158324)
2024-11-07 [email protected] Roll Flutter Engine from 076688d95818 to 8a963cfc134c (1 revision) (flutter/flutter#158304)
2024-11-07 [email protected] Roll Flutter Engine from 94dac953a95f to 076688d95818 (2 revisions) (flutter/flutter#158303)
2024-11-07 [email protected] Make leak tracking bots blocking. (flutter/flutter#157866)
2024-11-07 [email protected] Roll Flutter Engine from b36ca3319825 to 94dac953a95f (1 revision) (flutter/flutter#158297)
2024-11-06 [email protected] Roll Flutter Engine from 58ac1dadd69d to b36ca3319825 (9 revisions) (flutter/flutter#158295)
2024-11-06 [email protected] Added cusor control properties to CupertinoSearchTextField and tests (flutter/flutter#158240)
2024-11-06 [email protected] Fix flakiness in hot_reload_test.dart (flutter/flutter#158271)
2024-11-06 [email protected] Fix use of deprecated `buildDir` in Android templates/tests/examples (flutter/flutter#157560)
2024-11-06 [email protected] Roll Flutter Engine from f03f11300a9d to 58ac1dadd69d (5 revisions) (flutter/flutter#158283)
2024-11-06 [email protected] Roll pub packages (flutter/flutter#158281)
2024-11-06 [email protected] Delete firebase_android_embedding_v2_smoke_test (flutter/flutter#158223)
2024-11-06 [email protected] [web] fix --ab option for web benchmarks (flutter/flutter#154574)
2024-11-06 [email protected] excluding website-cms from critical pr triage (flutter/flutter#158220)
2024-11-06 [email protected] Add test for `image.frame_builder.0.dart` (flutter/flutter#158247)
2024-11-06 [email protected] Roll Packages from 7219431 to bb5a258 (6 revisions) (flutter/flutter#158267)

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] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App 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.

Long NavigationBar tab titles can't be padded from the sides of the screen
2 participants