Skip to content

Implement VisualDensity for text fields. - #51438

Merged
gspencergoog merged 3 commits into
flutter:masterfrom
gspencergoog:textfield_density
Feb 26, 2020
Merged

Implement VisualDensity for text fields.#51438
gspencergoog merged 3 commits into
flutter:masterfrom
gspencergoog:textfield_density

Conversation

@gspencergoog

@gspencergoog gspencergoog commented Feb 25, 2020

Copy link
Copy Markdown
Contributor

Description

This implements VisualDensity changes for text fields*. By default, the layout of the text field does not change.

If the ThemeData.visualDensity is set to a value other than zero, then the density of the UI will increase or decrease. See the VisualDensity docs for more information.

Here's a GIF of the compact, comfortable, and standard densities with an outline, underline, and multiline text field:

Density

(*In reality, the changes are on the InputDecorator class, not on the text field.)

I also fixed a problem that I think I found with _Decoration where it doesn't compare isDense or isCollapsed as part of its operator==.

Related Issues

Tests

  • Added tests for reduced visual density, as well as increased visual density.

Breaking Change

  • No, this is not a breaking change.

@gspencergoog gspencergoog added framework flutter/packages/flutter repository. See also f: labels. a: desktop Running on desktop labels Feb 25, 2020
@fluttergithubbot fluttergithubbot added p: material_ui material_ui package in flutter/packages c: contributor-productivity Team-specific productivity, code health, technical debt. labels Feb 25, 2020
justinmc
justinmc approved these changes Feb 26, 2020
View reviewed changes

@justinmc justinmc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Some questions about the larger state of InputDecorator though. How does this fit in with isDense and InputDecoration.collapsed? Do they all still have valid reasons to coexist as-is? There is an issue open about collapsed being broken right now, so I am thinking about how to fix that as well: #46160

Comment thread packages/flutter/lib/src/material/input_decorator.dart Outdated

@justinmc justinmc Feb 25, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: Maybe put spaces around the /.

@justinmc justinmc Feb 25, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I hope all of these calculations made sense to you by the way! I tried to clean up this method awhile back, but it's still fairly long and confusing.

@gspencergoog gspencergoog Feb 26, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

(fixed the spaces) Yeah, the names of the variables were pretty helpful in figuring out where to add the offset. It's still pretty confusing to me, but I think they're in the right places.

Comment thread packages/flutter/test/material/input_decorator_test.dart Outdated
@gspencergoog

gspencergoog commented Feb 26, 2020

Copy link
Copy Markdown
Contributor Author

isDense is a holdover from a customer request many years ago to be able to have a dense version of the text field that could fit into forms. At this point, I'm pretty sure it's just legacy.

I think collapsed is probably not needed anymore, although, unlike density, it does change the prefix padding, it looks like (and yes, it appears to be broken, looking at the sample).

The density API won't change the padding or the text size, it will just change some of the constraints and offsets. For text fields, it only affects vertical size.

@HansMuller

HansMuller commented Feb 26, 2020

Copy link
Copy Markdown
Contributor

isDense was a feature of the original Material spec, see https://material.io/archive/guidelines/components/text-fields.html#text-fields-layout. It continues to be part of the spec, see "Dense text fields" in https://material.io/components/text-fields, albeit without the precise information about layout.

@gspencergoog

gspencergoog commented Feb 26, 2020

Copy link
Copy Markdown
Contributor Author

Ahh, OK, I guess I'm mistaken about isDense.

@justinmc

justinmc commented Feb 26, 2020

Copy link
Copy Markdown
Contributor

@HansMuller @gspencergoog .collapsed was created at the same time that InputDecoration was created, without any specific notes about why: #9119

The issue about .collapsed being broken (#46160) has a screenshot of their app before the break:

I looked through the code for what isCollapsed actually does, and it seems that the only thing it does that can't be done with isDense is to remove the side padding:

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

Line 2272 in 64c5a43

contentPadding = decorationContentPadding ?? EdgeInsets.zero;

However, I think it could be done by manually setting contentPadding. So if that's all true, I think it would be ok to remove .collapsed.

@justinmc justinmc mentioned this pull request Feb 26, 2020
Closed
@gspencergoog
gspencergoog merged commit 9e744c5 into flutter:master Feb 26, 2020
@gspencergoog
gspencergoog deleted the textfield_density branch February 26, 2020 21:38
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Aug 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@justinmc justinmc justinmc approved these changes
@HansMuller HansMuller Awaiting requested review from HansMuller

Assignees

No one assigned

Labels

a: desktop Running on desktop c: contributor-productivity Team-specific productivity, code health, technical debt. framework flutter/packages/flutter repository. See also f: labels. p: material_ui material_ui package in flutter/packages

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@gspencergoog @HansMuller @justinmc @googlebot @fluttergithubbot