-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Expose affixes icon constraints in InputDecorationTheme #153089
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
Expose affixes icon constraints in InputDecorationTheme #153089
Conversation
9a4b5a0 to
ccc30cb
Compare
justinmc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
| /// 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. |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
ccc30cb to
f959ced
Compare
justinmc
left a comment
There was a problem hiding this 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 |
There was a problem hiding this comment.
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".
f44308e to
cefc816
Compare
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
## 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.
## 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.
## 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:  _____ 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:  After this PR: When `InputDecoration.suffixIconConstraints` is used the icon is correctly aligned:  <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.
Description
This PR makes the existing
InputDecoration.prefixIconConstraintsandInputDecoration.suffixIconConstraintsconfigurable from anInputDecorationTheme.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.