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 Tooltip default vertical padding #103395

Merged

Conversation

bleroux
Copy link
Contributor

@bleroux bleroux commented May 10, 2022

Description

This PR add a Tooltip default vertical padding for developper convenience.
see #86170 (comment)

@guidezpl
After investigating various solutions to not rely on vertical padding, I reached the conclusion that setting a default vertical padding is the best candidate :

  • Users can easily override the default value as there is already a padding property on Tooltip (without relying on vertical padding we should add a new property and manage edge cases where the user already set the padding property).
  • Before rendering, there is no simple way to know when a Text widget will wrap, so adding layout logic to increase Tooltip container height when the Text wraps will be complex and error-prone.

Related Issue

Fixes #86170

Tests

Update 3 tests.

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels May 10, 2022
@bleroux bleroux requested a review from guidezpl May 10, 2022 08:25
@bleroux bleroux force-pushed the fix_tooltip_vertical_padding branch from c7a2e1e to 6a87f87 Compare May 10, 2022 13:04
@guidezpl
Copy link
Member

Hi! I believe this is a breaking change and won't be able to land before preliminary work.

Also, this PR would benefit from a table with screenshots showing the resulting tooltip under various combinations (mobile/desktop, non-wrapping, wrapping).

@bleroux bleroux force-pushed the fix_tooltip_vertical_padding branch from 6a87f87 to 01e656c Compare May 18, 2022 14:05
@bleroux
Copy link
Contributor Author

bleroux commented May 18, 2022

Hey!
I tried to take several screenshots to demonstrate some combinations.

Code sample for screenshots
import 'package:flutter/material.dart';

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

class MyApp extends StatelessWidget {
  const MyApp({super.key, this.dark = false, this.useMaterial3 = true});

  final bool dark;
  final bool useMaterial3;

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      debugShowCheckedModeBanner: false,
      themeMode: dark ? ThemeMode.dark : ThemeMode.light,
      theme: ThemeData(
        useMaterial3: useMaterial3,
        brightness: Brightness.light,
        colorSchemeSeed: const Color(0xff6750a4),
      ),
      darkTheme: ThemeData(
        useMaterial3: useMaterial3,
        brightness: Brightness.dark,
        colorSchemeSeed: const Color(0xff6750a4),
      ),
      home: const Example(),
    );
  }
}

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

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: const Text('Tooltip Sample'),
      ),
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.spaceEvenly,
          children: <Widget>[
            Column(
              children: <Widget>[
                const SizedBox(height: 5),
                Container(
                  color: Colors.green[100],
                  child: const Tooltip(
                    message: "Non-wrapping Tooltip",
                    triggerMode: TooltipTriggerMode.tap,
                    showDuration: Duration(seconds: 5),
                    preferBelow: false,
                    child: Padding(
                      padding: EdgeInsets.all(8.0),
                      child: Text(
                        'Tap me to display\na non wrapping Tooltip',
                        textAlign: TextAlign.center,
                      ),
                    ),
                  ),
                ),
              ],
            ),
            Column(
              children: <Widget>[
                const SizedBox(height: 5),
                Container(
                  color: Colors.green[100],
                  child: const Tooltip(
                    message: "Wrapping\nTooltip",
                    triggerMode: TooltipTriggerMode.tap,
                    showDuration: Duration(seconds: 5),
                    preferBelow: false,
                    child: Padding(
                      padding: EdgeInsets.all(8.0),
                      child: Text(
                        'Tap me to display\na wrapping Tooltip',
                        textAlign: TextAlign.center,
                      ),
                    ),
                  ),
                ),
              ],
            ),
          ],
        ),
      ),
    );
  }
}

I also updated the PR with a vertical padding of 4.0 instead of 6.0 to make it less risky. The PR passed all customer_testing tests but I don't know if it might be a breaking change for some Google internal testing.

Desktop (Unbuntu) :
103395_ubuntu

Android :
103395_android

Copy link
Member

@guidezpl guidezpl left a comment

Choose a reason for hiding this comment

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

LGTM, nice. Let's see what testing says

@bleroux bleroux force-pushed the fix_tooltip_vertical_padding branch from 01e656c to 31f37de Compare May 18, 2022 19:55
@fluttergithubbot fluttergithubbot merged commit 4b67827 into flutter:master May 19, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 19, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request May 19, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request May 19, 2022
@bleroux bleroux deleted the fix_tooltip_vertical_padding branch May 20, 2022 10:37
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request May 21, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 30, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 30, 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.

Tooltip with long text is not wrapping nicely
3 participants