-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Fixes #11068 with error widget #118610
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
Fixes #11068 with error widget #118610
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Hello @KishanBusa8 and thank you for your contribution. Looks like its a much asked for feature! I'm happy to give you some feedback and try to get this merged. For some context, this feature has been proposed before #104165 . And there was some discussion around It seems to me that |
|
Hey, @Renzo-Olivares Thanks for your time. |
|
Hey, @Renzo-Olivares As per your comments I resolved all the changes and replace errorBuilder with errorWidget. you can check it on above commit. |
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.
Thank you for making those changes! Have a few more thoughts and small nits. Do you also think you can update the PR description and title? Thank you!
|
Hello @Renzo-Olivares Thank you for your time. |
|
If you take a look at the diff in the "Files" tab here on GitHub, you'll see that there are now a lot of changes happening that are to the formatting of other unrelated parts of the file. You'll want to undo those changes. In general it's a good practice to look at the exact diff you've submitted, and remove any changes that aren't intentional. (Several of the nits above were in this category.) You can do that in the GitHub UI, or you can do it with Git on your own computer. Doing it with Git become easier if you follow the advice in the wiki to not use merge: https://github.com/flutter/flutter/wiki/Tree-hygiene#using-git |
|
Hello @gnprice ohh I thought I enabled auto formate on save by mistake I’ll revert the commit and update you in some time. |
|
Cool. Everything in the PR now looks great as far as I'm concerned. |
|
Hello, @gnprice and @Renzo-Olivares Thank you, guys, for your help and support. |
|
This PR went with I would like to customise/stylise errors produced by the validator. How can I do it? |
|
@xvrh That's a good point. Currently if If this was an An Any thoughts on this @gnprice @KishanBusa8 @xvrh? |
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.
Hi @KishanBusa8 sorry for the delay on reviews, I have been out of office for the past week. I think everything looks good but I would suggest adding another test to confirm that everything is still being laid out properly, something similar to https://github.com/Renzo-Olivares/flutter/blob/229d70ea3e523396340542e8fd482521cc017a84/packages/flutter/test/material/input_decorator_test.dart#L1210
Thank you for your patience and the quick changes!
|
@Renzo-Olivares thanks for the consideration. |
|
Ah, excellent point.
I like this suggestion. |
In general the answer is: wait for additional reviews. See the Tree hygiene wiki page (which you should definitely read when contributing code to Flutter):
So waiting a few days is quite normal. Relatedly, there's generally no need to merge the master branch back into your PR. If there's an actual merge conflict, then that can be helpful for resolving it, but otherwise there's generally no benefit to doing so. |
|
I can think of several options to make it work with Option 1
var errorText = field.errorText;
var errorBuilder = field.widget.errorBuilder;
var effectiveDecoration = decoration.copyWith(
errorText: errorBuilder == null ? errorText : null,
error: errorText != null ? errorBuilder?.call(field.errorText) : null);
return TextField(decoration: effectiveDecoration);Option 2
This is the idea of @Renzo-Olivares above Option 3
Option 4
|
|
For option 1, it looks like if the That seems different from how it works now where even if I think this new API should follow the notion that |
|
I like option 2 (Renzo's) and I like the name validatorBuilder. Also I think that validator/validatorBuilder and error/errorBuilder should have assertions so that only one is allowed at a time, i.e. if you pass both validator and validatorBuilder then you get an error. |
justinmc
left a comment
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.
Just a few comments after looking at the validator discussion above.
| } | ||
|
|
||
| Widget _buildError() { | ||
| assert(widget.errorText != null); |
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.
I think it's still useful to assert here. If we don't then there will still be an error (on line 415 below), but this way the error is probably more clear.
assert(widget.error != null || widget.errorText != null);| if (_controller.isCompleted) { | ||
| _helper = null; | ||
| if (widget.errorText != null) { | ||
| if (widget.errorText != null || widget.error != null) { |
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.
Nit: It might be worth creating a getter for this since it's used pretty frequently.
bool get _hasError => widget.errorText != null || widget.error != null;| this.alignLabelWithHint, | ||
| this.constraints, | ||
| }) : assert(!(label != null && labelText != null), 'Declaring both label and labelText is not supported.'), | ||
| assert(!(error != null && errorText != null), 'Declaring both error and errorText is not supported.'), |
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 we add validatorBuilder, we should have an assertion similar to this in FormField for it and validator.
| }); | ||
|
|
||
| testWidgets('InputDecorator shows error widget', (WidgetTester tester) async { | ||
| // when error is defined |
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.
Capital letter at the start and period at the end.
| expect(tester.getSize(find.text('error')).height, 20.0); | ||
| expect(tester.getTopLeft(find.text('error')), const Offset(0.0, 56.0)); | ||
|
|
||
| // when errorText is defined |
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.
Here too: capital letter at the start and period at the end.
|
@KishanBusa8 Thanks for your contribution! Will you be able to continue working on this? It's totally fine if not; we can take it over (though it might take longer). I'm just asking because I'm trying to clean out our review queue. |
|
Since we haven't heard back from @KishanBusa8, let's close this in favor of #129275. Thanks for helping us get to a solution regardless! |
This PR is made for allows to add an error widget in InputDecoration to add your own error wIdget to Textfield.
List which issues are fixed by this PR. You must list at least one issue.
Fixes #11068
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.