-
Notifications
You must be signed in to change notification settings - Fork 29.1k
Deprecate maxLengthEnforced
for text fields
#72043
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deprecate maxLengthEnforced
for text fields
#72043
Conversation
Hmm, the commit seems not rolled in yet. Any ideas? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR looks good to me. Thank you for your patience and the effort put into the series of pull requests!
@@ -805,7 +820,7 @@ class _CupertinoTextFieldState extends State<CupertinoTextField> with Restoratio | |||
FocusNode get _effectiveFocusNode => widget.focusNode ?? (_focusNode ??= FocusNode()); | |||
|
|||
MaxLengthEnforcement get _effectiveMaxLengthEnforcement => widget.maxLengthEnforcement | |||
?? LengthLimitingTextInputFormatter.inferredDefaultMaxLengthEnforcement; | |||
?? LengthLimitingTextInputFormatter.getDefaultMaxLengthEnforcement(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like the new name 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this related to the deprecation at all or just an improvement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's an improvement, that the style guide say we preferred getXXX
for method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just one nit about an optional parameter.
I see that you updated the gallery (flutter/gallery#358) but it's still failing here. I'm not sure what needs to happen for the tests to get updated here...
@@ -432,11 +433,13 @@ class LengthLimitingTextInputFormatter extends TextInputFormatter { | |||
/// [MaxLengthEnforcement.truncateAfterCompositionEnds]. These platforms | |||
/// allow the composition to exceed by default. | |||
/// {@endtemplate} | |||
static MaxLengthEnforcement get inferredDefaultMaxLengthEnforcement { | |||
static MaxLengthEnforcement getDefaultMaxLengthEnforcement({ | |||
TargetPlatform? platform, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be better as an optional parameter with a default value instead?
[TargetPlatform platform = defaultTargetPlatform],
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remember it correctly, defaultTargetPlatform
is a getter, so it cannot be the default value for arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A position optional param or a named optional param both SGTM, I'll take your suggestion.
@@ -805,7 +820,7 @@ class _CupertinoTextFieldState extends State<CupertinoTextField> with Restoratio | |||
FocusNode get _effectiveFocusNode => widget.focusNode ?? (_focusNode ??= FocusNode()); | |||
|
|||
MaxLengthEnforcement get _effectiveMaxLengthEnforcement => widget.maxLengthEnforcement | |||
?? LengthLimitingTextInputFormatter.inferredDefaultMaxLengthEnforcement; | |||
?? LengthLimitingTextInputFormatter.getDefaultMaxLengthEnforcement(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this related to the deprecation at all or just an improvement?
After the flutter/tests#72 get's merged, customer tests will be succeed. |
Description
Further work for #68086 . Deprecate
maxLengthEnforced
for text fields, and some nit picking update.Tests
I added the following tests:
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.///
).flutter analyze --flutter-repo
) does not report any problems on my PR.Breaking Change
Did any tests fail when you ran them? Please read Handling breaking changes.