Skip to content

Conversation

@bleroux
Copy link
Contributor

@bleroux bleroux commented Aug 8, 2024

Description

This PR makes the existing InputDecoration.prefixIconConstraints and InputDecoration.suffixIconConstraints configurable from an InputDecorationTheme.

Related Issue

Related to #138691 (this is needed before providing a fix or a workaround for it).

Tests

Update and split one existing test into two different tests.
Update the existing test related to debugFillDescription by adding all the non tested properties.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Aug 8, 2024
@bleroux bleroux force-pushed the expose_affix_constraints_in_Input_decoration_theme branch 2 times, most recently from 9a4b5a0 to ccc30cb Compare August 8, 2024 14:26
@bleroux bleroux requested a review from justinmc August 8, 2024 15:10
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Comment on lines 4075 to 4078
/// This property is particularly useful for getting the decoration's height
/// less than 48px. This can be achieved by setting [isDense] to true and
/// setting the constraints' minimum height and width to a value lower than
/// 48px.
Copy link
Contributor

Choose a reason for hiding this comment

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

Good call writing this out explicitly, I think it's a common problem.

/// [InputDecoration.suffixIcon].
///
/// This property is particularly useful for getting the decoration's height
/// less than 48px. This can be achieved by setting [isDense] to true and
Copy link
Contributor

Choose a reason for hiding this comment

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

Super nit: Should you say "the minimum tappable height" instead of 48px, to avoid these comments being wrong if the constant changes? I'm not sure, maybe that's more confusing that just keeping it as 48px.

Copy link
Contributor Author

@bleroux bleroux Aug 9, 2024

Choose a reason for hiding this comment

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

Good call! It is not that I'm worried about a change on the constant (because if it changes we will have to update many comments 😅 ) but what I like with your suggestion is that it is always correct, compared to "48px" which only applies when visual density is standard.
I upated the PR to use "the minimum tappable height" and I also add a mention to visual density. Can you check it and maybe rephrase it?

@bleroux bleroux force-pushed the expose_affix_constraints_in_Input_decoration_theme branch from ccc30cb to f959ced Compare August 9, 2024 13:02
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

One very minor nit, otherwise the phrasing and everything else looks good!

/// less than 48px. This can be achieved by setting [isDense] to true and
/// setting the constraints' minimum height and width to a value lower than
/// 48px.
/// less than the minimum tappable height (which is 48px when the visual
Copy link
Contributor

Choose a reason for hiding this comment

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

Super nit: Wherever you say "minimum tappable height", I think it should be changed to "minimum tappable size". I think it makes it more logically consistent when you refer to "height and width" later in this paragraph:

- setting the constraints' minimum height and width to a value lower than the minimum tappable height.
+ setting the constraints' minimum height and width to a value lower than the minimum tappable size.

I think that sounds better so you're not comparing "height and width" to "height".

@bleroux bleroux force-pushed the expose_affix_constraints_in_Input_decoration_theme branch from f44308e to cefc816 Compare August 9, 2024 21:05
@bleroux bleroux added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 9, 2024
@auto-submit auto-submit bot merged commit 5609019 into flutter:master Aug 9, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 10, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Aug 10, 2024
Manual roll requested by [email protected]

flutter/flutter@76107bd...b12d861

2024-08-10 [email protected] Move windows mokey tests to prod (flutter/flutter#153203)
2024-08-10 [email protected] Roll Flutter Engine from 2f577d8e3fa2 to 3acd373923c9 (7 revisions) (flutter/flutter#153210)
2024-08-09 [email protected] Also run Linux_android_emu tests on API level 34 image (flutter/flutter#153175)
2024-08-09 [email protected] Expose affixes icon constraints in InputDecorationTheme (flutter/flutter#153089)
2024-08-09 [email protected] Spell check range error (flutter/flutter#153055)
2024-08-09 [email protected] Roll Flutter Engine from 4d5b1df96fc9 to 2f577d8e3fa2 (4 revisions) (flutter/flutter#153191)
2024-08-09 [email protected] Roll Flutter Engine from c9ec468c7592 to 4d5b1df96fc9 (6 revisions) (flutter/flutter#153182)
2024-08-09 [email protected] Roll Packages from bb797b9 to f7b1256 (5 revisions) (flutter/flutter#153180)
2024-08-09 [email protected] Move Cupertino focus constants to cupertino/constants.dart (flutter/flutter#153115)
2024-08-09 [email protected] Fix tests expectations regarding new lines. (flutter/flutter#153174)
2024-08-09 [email protected] Shift tests on moto g4 on Windows hosts to mokey in staging (flutter/flutter#153167)
2024-08-09 [email protected] Make ios_deploy_test.dart more robust (flutter/flutter#153147)
2024-08-09 [email protected] Roll Flutter Engine from e50fd307b6f7 to c9ec468c7592 (2 revisions) (flutter/flutter#153145)
2024-08-09 [email protected] Roll Flutter Engine from 05208896830a to e50fd307b6f7 (5 revisions) (flutter/flutter#153143)

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
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
@bleroux bleroux deleted the expose_affix_constraints_in_Input_decoration_theme branch August 10, 2024 15:15
DBowen33 pushed a commit to DBowen33/flutter that referenced this pull request Aug 16, 2024
## Description

This PR makes the existing `InputDecoration.prefixIconConstraints` and `InputDecoration.suffixIconConstraints` configurable from an `InputDecorationTheme`.

## Related Issue

Related to flutter#138691 (this is needed before providing a fix or a workaround for it).

## Tests

Update and split one existing test into two different tests.
Update the existing test related to debugFillDescription by adding all the non tested properties.
Buchimi pushed a commit to Buchimi/flutter that referenced this pull request Sep 2, 2024
## Description

This PR makes the existing `InputDecoration.prefixIconConstraints` and `InputDecoration.suffixIconConstraints` configurable from an `InputDecorationTheme`.

## Related Issue

Related to flutter#138691 (this is needed before providing a fix or a workaround for it).

## Tests

Update and split one existing test into two different tests.
Update the existing test related to debugFillDescription by adding all the non tested properties.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 11, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 12, 2024
github-merge-queue bot pushed a commit that referenced this pull request Jan 15, 2025
## Description

This PR fixes DropdownMenu arrow icon position when
`InputDecoration.isCollapsed` is set to true and
`InputDecoration.suffixConstraints` is set to a smaller value than the
min interactive height.

It makes it possible to use collapsed `DropdownMenu` such as:


![image](https://github.com/user-attachments/assets/145c2a0b-a638-4226-ae29-0e21bb9d4bb0)

_____

Before this PR and #153089,
`InputDecoration.isCollapsed` had no impact on the `DropdownMenu` height
and there was no solution to reduce the height because its minimum
height is enforced by the `IconButton` (arrow down or up) which is part
of the `DropdownMenu`.

Since #153089, the height can be
reduce with `InputDecoration.suffixIconConstraints` but it results in
the icon being misaligned:


![image](https://github.com/user-attachments/assets/2a4d99bc-c26d-454b-8e62-dd8828789ae5)


After this PR:

When `InputDecoration.suffixIconConstraints` is used the icon is
correctly aligned:


![image](https://github.com/user-attachments/assets/145c2a0b-a638-4226-ae29-0e21bb9d4bb0)


<details><summary>Code sample</summary>

```dart
import 'package:flutter/material.dart';

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

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

  @OverRide
  Widget build(BuildContext context) {
    return const MaterialApp(
      title: 'Flutter Demo',
      home: MyHomePage(title: 'Flutter Demo Home Page'),
    );
  }
}

class MyHomePage extends StatefulWidget {
  const MyHomePage({super.key, required this.title});
  final String title;

  @OverRide
  State<MyHomePage> createState() => _MyHomePageState();
}

class _MyHomePageState extends State<MyHomePage> {
  @OverRide
  Widget build(BuildContext context) {
    return const Scaffold(
      body: Center(
        child: Center(
          child: DropdownMenu<String>(
            dropdownMenuEntries: [
              DropdownMenuEntry(label: 'Item 1', value: '1'),
              DropdownMenuEntry(label: 'Item 2', value: '2'),
            ],
            inputDecorationTheme: InputDecorationTheme(
              contentPadding: EdgeInsets.fromLTRB(5, 0, 5, 0),
              isCollapsed: true,
              // Usable since #153089.
              suffixIconConstraints: BoxConstraints(minHeight: 24, maxHeight: 24),
              filled: true,
            ),
          ),
        ),
      ),
    );
  }
}

``` 

</details> 

## Related Issue

Fixes [DropdownMenu inputDecoration isCollapsed property does not reduce
vertical spacing](#138691)

## Tests

Adds 2 tests.
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.

2 participants