Skip to content

Conversation

@KishanBusa8
Copy link

@KishanBusa8 KishanBusa8 commented Jan 17, 2023

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

  • 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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Jan 17, 2023
@google-cla
Copy link

google-cla bot commented Jan 17, 2023

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.

@HansMuller HansMuller added the a: text input Entering text in a text field or keyboard related problems label Jan 20, 2023
@Renzo-Olivares
Copy link
Contributor

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 errorWidget vs errorBuilder.

It seems to me that errorWidget covers all of the use cases that we might want to cover, is there any added functionality given by a builder? I think a user can either provide an errorText or an errorWidget and not both. If a user wants custom text in their error, then they can make a custom text widget with their changes and provide that as the errorWidget. Is there any cases this doesn't cover?

@KishanBusa8
Copy link
Author

Hey, @Renzo-Olivares Thanks for your time.
I think you are right we might need to use errorWidget instead of the builder. I'll add this change in the next commit.
Thanks!

@KishanBusa8
Copy link
Author

Hey, @Renzo-Olivares As per your comments I resolved all the changes and replace errorBuilder with errorWidget. you can check it on above commit.
Thanks!

Copy link
Contributor

@Renzo-Olivares Renzo-Olivares left a 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!

@KishanBusa8 KishanBusa8 changed the title Fixes #11068 with errorWidgetBuilder Fixes #11068 with errorWidget Feb 16, 2023
@KishanBusa8
Copy link
Author

Hello @Renzo-Olivares Thank you for your time.
I have updated the PR title and description as per your suggestion and also completed the changes which you have suggest. You can check the new commit and Please let me know if we need any fixes.
I really appreciate your help.
Thanks :)

@KishanBusa8 KishanBusa8 changed the title Fixes #11068 with errorWidget Fixes #11068 with error widget Feb 16, 2023
@gnprice
Copy link
Member

gnprice commented Feb 24, 2023

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

@KishanBusa8
Copy link
Author

Hello @gnprice ohh I thought I enabled auto formate on save by mistake I’ll revert the commit and update you in some time.
Sorry about that..:)

@gnprice
Copy link
Member

gnprice commented Feb 24, 2023

Cool. Everything in the PR now looks great as far as I'm concerned.

@KishanBusa8
Copy link
Author

Hello, @gnprice and @Renzo-Olivares Thank you, guys, for your help and support.
If all is good then what's the next step as this is my first PR I don't know.
Thanks!

@xvrh
Copy link
Contributor

xvrh commented Feb 26, 2023

This PR went with errorWidget instead of errorBuilder. How will it not work with TextFormField.validator?

I would like to customise/stylise errors produced by the validator. How can I do it?

@Renzo-Olivares
Copy link
Contributor

@xvrh That's a good point. Currently if validator is defined then the String from the validator overrides the InputDecoration.errorText.

If this was an errorBuilder instead that would mean that the widget needs to be linked to the errorText to grab the validators text.

An errorBuilder makes sense to me, but I also think theres another option. Maybe adding a validatorBuilder that overrides InputDecoration.error in the same vein that validator overrides InputDecoration.errorText.

Any thoughts on this @gnprice @KishanBusa8 @xvrh?

Copy link
Contributor

@Renzo-Olivares Renzo-Olivares left a 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!

@xvrh
Copy link
Contributor

xvrh commented Feb 27, 2023

@Renzo-Olivares thanks for the consideration.
I think the best api is to have a Widget Function(String) errorBuilder property on InputDecoration. This callback allows to convert any InputDecoration.errorText to a widget.
It gives the developer a way to stylise the error message produced by the validator (ie. wrap the Text in a DecoratedBox). This is one of the most important use case for me.

@gnprice
Copy link
Member

gnprice commented Feb 27, 2023

Ah, excellent point.

I think the best api is to have a Widget Function(String) errorBuilder property on InputDecoration. This callback allows to convert any InputDecoration.errorText to a widget.

I like this suggestion.

@gnprice
Copy link
Member

gnprice commented Feb 27, 2023

Thank you, guys, for your help and support.
If all is good then what's the next step as this is my first PR I don't know.

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

Expect that a new patch will be reviewed within two weeks, unless it is fixing a P0 bug in which case it should be reviewed the same day. If it has not been reviewed in that timeframe, reach out on Chat. Remember that reviewers are human beings with additional professional and personal responsibilities.

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.

@xvrh
Copy link
Contributor

xvrh commented Feb 27, 2023

I can think of several options to make it work with TextFormField.validator

Option 1

  • Add Widget error in InputDecoration (this PR)
  • Add Widget Function(String)? errorBuilder in FormField
  • Apply the errorBuilder to the error message produced by the validator like this:
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

  • Add Widget error in InputDecoration (this PR)
  • Add a validator variant: Widget? Function(String? input)? validatorBuilder in FormField. It allows to replace the classical validator with a version that can return a Widget instead of a string.

This is the idea of @Renzo-Olivares above

Option 3

  • Add Widget error in InputDecoration (this PR)
  • Change the type of validator to something like Object/*String|Widget*/? Function(String? input).

Option 4

  • Add Widget Function(String)? errorBuilder on InputDecoration which when not null is called to convert errorText to a Widget.

@Renzo-Olivares
Copy link
Contributor

Renzo-Olivares commented Mar 1, 2023

For option 1, it looks like if the errorText is null (this can be because no validator is defined or no InputDecoration.errorText is defined) there will be no error widget displayed even if an error is defined in the InputDecoration given to TextFormField. Let me know if i'm missing something here.

That seems different from how it works now where even if errorText is null in the InputDecoration given to TextFormField, the validators error text would still show up/ if there is no validator, but an errorText is given then the errorText still shows up.

I think this new API should follow the notion that validator is an overrider for InputDecoration.errorText, so validatorBuilder should be an overrider for the InputDecoration.error. cc @justinmc if you had any thoughts on this API discussion.

@justinmc
Copy link
Contributor

justinmc commented Mar 1, 2023

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.

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.

Just a few comments after looking at the validator discussion above.

}

Widget _buildError() {
assert(widget.errorText != null);
Copy link
Contributor

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) {
Copy link
Contributor

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.'),
Copy link
Contributor

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

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

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.

@Hixie
Copy link
Contributor

Hixie commented May 9, 2023

@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.

@justinmc
Copy link
Contributor

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!

@justinmc justinmc closed this Jun 26, 2023
auto-submit bot pushed a commit that referenced this pull request Jun 28, 2023
## Description

This PR is a fork of #118610 with some changes (mainly applying @justinmc comments).

~~This can be used by KishanBusa8 to update #118610 or can become a non WIP PR if KishanBusa8 does not respond or can not work on the update.~~

## Related Issue

fixes #11068

## Tests

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

Labels

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.

InputDecorator 'errorText' should have 'textAlign' property

7 participants