Skip to content

Conversation

Piinks
Copy link
Contributor

@Piinks Piinks commented Apr 30, 2020

Description

This PR introduces a temporary opt-in flag as the first step of migrating TextField to be able to assert debugCheckHasMaterialLocalizations, which has been disabled for some time. Making the new change opt-in allows developers the opportunity to experience the changes ahead of time so that they can fix any issues with the app resulting from the change or fix any broken tests that do not expect details in the new implementation.

Example migration steps

  1. Set all TextField instances in app to have canAssertMaterialLocalizations be true.
TextField(
  canAssertMaterialLocalizations: true, // add this line
  // ...
)
  1. Examine all screens with TextField to ensure that Material Localizations are present, make sure existing unit tests are not breaking. If something is broken, fix it. An example of a test that might fail is if a unit test was using a TextField without a MaterialApp or Localizations ancestor. If the issue cannot be fixed, it may not be an intended change, so file an issue to flutter/flutter on GitHub.

  2. A new PR will be introduced in the future to set TextField.canAssertMaterialLocalizations to true by default. This will default everybody's TextField to the latest design by default. If developers have properly migrated in step 2, this shouldn't cause any issues to your app. This would be the opportunity to remove the canAssertMaterialLocalizations parameter from your application since it is true by default now and it will be removed in a subsequent PR.

TextField(
  // canAssertMaterialLocalizations: true, // remove this line, since it should be true by default at this point.
  // ...
)
  1. Finally, a final PR will remove the canAssertMaterialLocalizations parameter altogether. If you removed the canAssertMaterialLocalizations parameter from your TextField instances in step 3, this should not affect your application.

Tests

Updated remaining test affected by the future default behavior.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

  • No, no existing tests failed, so this is not a breaking change.
  • Yes, this is a breaking change. Non-breaking phased plan
    • I got input from the developer relations team
    • I wrote a migration guide: for when we get to step 3

@Piinks Piinks added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. a: internationalization Supporting other languages or locales. (aka i18n) labels Apr 30, 2020
@Piinks Piinks changed the title Enable debugCheckHasMaterialLocalizations assertion on TextField [WIP] Enable debugCheckHasMaterialLocalizations assertion on TextField Apr 30, 2020
@Piinks
Copy link
Contributor Author

Piinks commented Apr 30, 2020

@HansMuller @justinmc @jonahwilliams This is another (simple?) ToDo resolution. It's slightly breaking, so I'll probably do the same as I did for #55336. Is there any other reason why we should not resolve this? I haven't found any reference in the git history to indicate otherwise.

@Piinks Piinks added c: API break Backwards-incompatible API changes work in progress; do not review labels Apr 30, 2020
@jonahwilliams
Copy link
Contributor

I think I added that, saw that it broke a bunch of tests and got scared that it was a breaking change (TM)

@justinmc
Copy link
Contributor

justinmc commented May 1, 2020

I see no reason not to go forward with this 👍

@Piinks Piinks marked this pull request as ready for review May 1, 2020 22:06
@Piinks Piinks changed the title [WIP] Enable debugCheckHasMaterialLocalizations assertion on TextField [WIP] Step 1 of 3: Enable debugCheckHasMaterialLocalizations assertion on TextField May 4, 2020
@Piinks Piinks removed the c: API break Backwards-incompatible API changes label May 4, 2020
@Piinks Piinks changed the title [WIP] Step 1 of 3: Enable debugCheckHasMaterialLocalizations assertion on TextField [WIP] Step 1 of 3: Add opt-in for debugCheckHasMaterialLocalizations assertion on TextField May 5, 2020
@Piinks Piinks changed the title [WIP] Step 1 of 3: Add opt-in for debugCheckHasMaterialLocalizations assertion on TextField Step 1 of 3: Add opt-in for debugCheckHasMaterialLocalizations assertion on TextField May 7, 2020
@Piinks
Copy link
Contributor Author

Piinks commented May 7, 2020

This is ready for review. CI is failing because I have not tagged the deprecation notices with a build, but I'll do that before this lands.

@Piinks Piinks requested review from HansMuller and justinmc May 8, 2020 18:52
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Is there anywhere in our codebase that should be updated to set canAssertMaterialLocalizations to true right now? Or maybe things like the gallery?

@Piinks
Copy link
Contributor Author

Piinks commented May 13, 2020

I don't think so. When I initially opened this I set it to true by default to see what would happen. Only that one test broke, I'll double check though before landing this first step in the migration.

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

Labels

a: internationalization Supporting other languages or locales. (aka i18n) a: text input Entering text in a text field or keyboard related problems f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants