Skip to content

TextField should support disabled input text style - #119216

Merged
Renzo-Olivares merged 19 commits into
flutter:masterfrom
Renzo-Olivares:disabled-input
Mar 17, 2023
Merged

TextField should support disabled input text style#119216
Renzo-Olivares merged 19 commits into
flutter:masterfrom
Renzo-Olivares:disabled-input

Conversation

@Renzo-Olivares

@Renzo-Olivares Renzo-Olivares commented Jan 26, 2023

Copy link
Copy Markdown
Contributor

Fixes #118568

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • All existing and new tests are passing.

@flutter-dashboard flutter-dashboard Bot added a: text input Entering text in a text field or keyboard related problems p: material_ui material_ui package in flutter/packages framework flutter/packages/flutter repository. See also f: labels. labels Jan 26, 2023
justinmc
justinmc approved these changes Jan 28, 2023
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 👍

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

@justinmc justinmc Jan 28, 2023

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: Could this be private?

@Renzo-Olivares Renzo-Olivares Jan 30, 2023

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.

Mostly did this because input_decorator does the same here

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

Line 2136 in 92df6b4

Set<MaterialState> get materialState {
. Not sure if that was intentional or not but I don't see why it can't be private.

@LongCatIsLooong

LongCatIsLooong commented Jan 30, 2023

Copy link
Copy Markdown
Contributor

I think in #118568 the suggestion was that style should support MaterialStateProperty?

@Renzo-Olivares

Renzo-Olivares commented Jan 30, 2023

Copy link
Copy Markdown
Contributor Author

I think in #118568 the suggestion was that style should support MaterialStateProperty?

You are right! Should be using MaterialStateProperty now.

justinmc
justinmc approved these changes Jan 30, 2023
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, thanks for the changes. I agree it's probably safer to make _materialState private for now. We can easily open it up if people ask for it for some reason.

@Renzo-Olivares

Renzo-Olivares commented Jan 31, 2023

Copy link
Copy Markdown
Contributor Author

CC @Hangyujin Wondering if you had any thoughts on this implementation since you migrated TextField to Material3.

hannah-hyj
hannah-hyj approved these changes Jan 31, 2023
View reviewed changes

@hannah-hyj hannah-hyj left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@Renzo-Olivares Renzo-Olivares added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 31, 2023
@auto-submit auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jan 31, 2023
@auto-submit

auto-submit Bot commented Jan 31, 2023

Copy link
Copy Markdown
Contributor

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

@Renzo-Olivares

Renzo-Olivares commented Jan 31, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Looks like failing tests are related to users implementing their own method to have a disabled input style in a text field. I'll need to go through the tests and see whats the best way forward.

@Renzo-Olivares

Renzo-Olivares commented Feb 16, 2023

Copy link
Copy Markdown
Contributor Author

This one needs another review because a few changes have been made. Mostly refactoring and using tokens and generators. Thank you!

@Renzo-Olivares

Renzo-Olivares commented Feb 28, 2023

Copy link
Copy Markdown
Contributor Author

I changed the enabled default color back to Theme.of(context).textTheme.bodyLarge.color because there were many failing scubas (1000+) as apps are expecting that textTheme color is what is used to override the default input color. In Material3 this was changed to onSurface. Looks like there is an issue tracking the migration of default text styles to use onSurface as the default color #107686. I can give that a shot in a separate PR.

justinmc
justinmc approved these changes Feb 28, 2023
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 👍

Comment thread dev/tools/gen_defaults/lib/text_field_template.dart Outdated

@justinmc justinmc Feb 28, 2023

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'm totally unaware of this whole Material 3 token thing, is there somewhere I can read about it?

@Renzo-Olivares Renzo-Olivares Feb 28, 2023

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.

This is the resource which explains it at a high level https://github.com/flutter/flutter/blob/master/dev/tools/gen_defaults/README.md . I mostly just looked into the other template files to get an idea of how to write this one. Not sure if there is a better resource.

@justinmc justinmc Mar 1, 2023

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.

Thank you!

@Renzo-Olivares

Renzo-Olivares commented Mar 17, 2023

Copy link
Copy Markdown
Contributor Author

Merging this since all failures are expected.

@Renzo-Olivares
Renzo-Olivares merged commit 358d79c into flutter:master Mar 17, 2023
@HansMuller HansMuller mentioned this pull request Mar 17, 2023
Closed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 18, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 18, 2023
@engine-flutter-autoroll
358d79c TextField should support disabled input text style (flutter/f…
5913bb5 
…lutter#119216)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
@engine-flutter-autoroll
358d79c TextField should support disabled input text style (flutter/f…
7fa4b35 
…lutter#119216)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
@engine-flutter-autoroll
358d79c TextField should support disabled input text style (flutter/f…
d423e9b 
…lutter#119216)
@slaci slaci mentioned this pull request Aug 9, 2023
Closed
2 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@justinmc justinmc justinmc approved these changes
@LongCatIsLooong LongCatIsLooong Awaiting requested review from LongCatIsLooong
@hannah-hyj hannah-hyj Awaiting requested review from hannah-hyj

Assignees

No one assigned

Labels

a: text input Entering text in a text field or keyboard related problems 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.

TextFields don't support disabled input text styling

4 participants

@Renzo-Olivares @LongCatIsLooong @justinmc @hannah-hyj