Skip to content

InputDecorator.hintStyle should use tokens when using material3 - #120910

Closed
Renzo-Olivares wants to merge 10 commits into
flutter:masterfrom
Renzo-Olivares:hint-text-fix
Closed

InputDecorator.hintStyle should use tokens when using material3#120910
Renzo-Olivares wants to merge 10 commits into
flutter:masterfrom
Renzo-Olivares:hint-text-fix

Conversation

@Renzo-Olivares

@Renzo-Olivares Renzo-Olivares commented Feb 16, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

This adds placeholder tokens to the input_decorator_template so hintStyle matches m3 spec.

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 p: material_ui material_ui package in flutter/packages framework flutter/packages/flutter repository. See also f: labels. labels Feb 16, 2023
@flutter-dashboard

flutter-dashboard Bot commented Feb 16, 2023

Copy link
Copy Markdown

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@flutter-dashboard flutter-dashboard Bot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Feb 16, 2023
@Renzo-Olivares

Renzo-Olivares commented Feb 16, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

The changes to switch are unexpected and causing some broken tests. These appear when running dart dev/tools/gen_defaults/bin/gen_defaults.dart on master without any changes as well Renzo-Olivares@a4849c0.

@QuncCccccc QuncCccccc mentioned this pull request Feb 16, 2023
Merged
7 of 8 tasks
@Renzo-Olivares

Renzo-Olivares commented Feb 16, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Should be good to go now.

QuncCccccc
QuncCccccc approved these changes Feb 16, 2023
View reviewed changes

@QuncCccccc QuncCccccc 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/input_decorator_template.dart Outdated

@QuncCccccc QuncCccccc Feb 16, 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: might be better to add a space after "color:"

@Renzo-Olivares Renzo-Olivares Feb 16, 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.

good catch! thanks

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

@QuncCccccc QuncCccccc Feb 16, 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: same here:)

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

@QuncCccccc QuncCccccc Feb 16, 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.

Just wonder if we also need to check the hint style for the outlined text field? Because I saw both types are using same hint style.

@Renzo-Olivares Renzo-Olivares Feb 16, 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.

Looking at the other hint tests in this file, looks like none of them set the filled property so should be the default (false). I think if they use the same hintStyle we should be alright since this is just checking that.

@QuncCccccc QuncCccccc Feb 16, 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 see. Thx!

@Renzo-Olivares Renzo-Olivares mentioned this pull request Feb 17, 2023
Merged
8 tasks done
hannah-hyj
hannah-hyj approved these changes Feb 18, 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

@Hixie

Hixie commented May 16, 2023

Copy link
Copy Markdown
Contributor

@Renzo-Olivares is this still on your radar?

@Renzo-Olivares

Renzo-Olivares commented May 18, 2023

Copy link
Copy Markdown
Contributor Author

@Hixie This is on my radar, i'll check on google testing.

@github-actions github-actions Bot removed framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. labels May 30, 2023
@github-actions github-actions Bot added framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. labels Jun 13, 2023
@Renzo-Olivares

Renzo-Olivares commented Jun 20, 2023

Copy link
Copy Markdown
Contributor Author

Around 13 unexpected scubas I still have left to investigate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@QuncCccccc QuncCccccc QuncCccccc approved these changes
@hannah-hyj hannah-hyj hannah-hyj approved these changes

Assignees

No one assigned

Labels

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.

4 participants

@Renzo-Olivares @Hixie @QuncCccccc @hannah-hyj