Skip to content

InputDecorator.hintStyle should use tokens when using material3#120910

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

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

Conversation

@Renzo-Olivares
Copy link
Contributor

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

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 f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Feb 16, 2023
@flutter-dashboard
Copy link

flutter-dashboard bot commented Feb 16, 2023

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
Copy link
Contributor Author

Renzo-Olivares commented Feb 16, 2023 •
edited
Loading

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
8 tasks
@Renzo-Olivares
Copy link
Contributor Author

Renzo-Olivares commented Feb 16, 2023 •
edited
Loading

Should be good to go now.

QuncCccccc
QuncCccccc approved these changes Feb 16, 2023
View reviewed changes
Copy link
Contributor

@QuncCccccc QuncCccccc left a comment

Choose a reason for hiding this comment

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

LGTM!

dev/tools/gen_defaults/lib/input_decorator_template.dart Outdated
Copy link
Contributor

@QuncCccccc QuncCccccc Feb 16, 2023

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:"

Copy link
Contributor Author

@Renzo-Olivares Renzo-Olivares Feb 16, 2023

Choose a reason for hiding this comment

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

good catch! thanks

dev/tools/gen_defaults/lib/input_decorator_template.dart Outdated
Copy link
Contributor

@QuncCccccc QuncCccccc Feb 16, 2023

Choose a reason for hiding this comment

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

nit: same here:)

packages/flutter/test/material/input_decorator_test.dart Outdated
Copy link
Contributor

@QuncCccccc QuncCccccc Feb 16, 2023

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.

Copy link
Contributor Author

@Renzo-Olivares Renzo-Olivares Feb 16, 2023

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.

Copy link
Contributor

@QuncCccccc QuncCccccc Feb 16, 2023

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
hannah-hyj
hannah-hyj approved these changes Feb 18, 2023
View reviewed changes
Copy link
Member

@hannah-hyj hannah-hyj left a comment

Choose a reason for hiding this comment

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

LGTM

@Hixie
Copy link
Contributor

Hixie commented May 16, 2023

@Renzo-Olivares is this still on your radar?

@Renzo-Olivares
Copy link
Contributor Author

Renzo-Olivares commented May 18, 2023

@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
Copy link
Contributor Author

Renzo-Olivares commented Jun 20, 2023

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. f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

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