-
Notifications
You must be signed in to change notification settings - Fork 28.9k
feat: add errorBuilder property to customize errorText appearance #156275
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
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
@goderbauer Can I also deprecate |
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.
@bleroux I wonder if you have any thoughts after working on adding .error
(#129275)? Should we consider deprecating .error
and using only .errorBuilder
?
The API has a lot of overlap here, between validator, error, errorText, and errorBuilder. Ideally we would trim it down, but I'm hoping to get a round to a bigger Form refactor in the future. So in the meantime I think it makes sense to add errorBuilder.
/// Use [errorBuilder] instead of [error] if you need to show the | ||
/// validator error but at the same time also customize the error widget. | ||
/// | ||
/// Only one of [error] or [errorBuilder] can be specified. |
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.
Somewhere I think we should mention that errorText comes from TextFormField.validator. Is here the best place?
See also:
- [TextFormField.validator], which passes its validation error as [InputDecoration.errorText] through to
errorBuilder
.
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.
This is good now 👍
@justinmc If we stay strictly look at the If the use case is to show a custom message, one will have to provide both Looking at the mentioned issues it seems to be a problem with
I think it would be better to add
We also have I'm 💯% for "a bigger Form refactor in the future"! |
The use case is that I wanna show error text from validator, with some added decorations. Right now I can not do that because:
After merging this PR:
|
@MohiuddinM |
For me the correct location is definitely |
I added it in |
Having the possibility to customize the errorBuilder at the |
@bleroux Ah I agree with you about this being a TextFormField problem, great point. I want to say that the InputDecorationTheme is a separate problem and we should move forward with something like @bleroux's #157087. Separately I think we could still add InputDecorationTheme.errorBuilder. Have we put a WidgetBuilder into a Theme before? Edit: Yes, for example ButtonStyle.backgroundBuilder. Thanks @QuncCccccc for helping me find that. |
Having |
@MohiuddinM Would you be on board with modifying this PR so that errorBuilder is on TextFormField instead of InputDecoration? Then we can think about theming after that's merged. |
I do not think that is a good idea, right now I am dealing with 40 form fields and using the same InputDecoration (because the UI and theeming for all are the same, only labels are different). This is the same case most apps deal with, where text inputs need to look the same and hence share InputDecoration. With this PR, I just have to add one more line (along with other UI adaptations, e.g., border styles, label styles, etc.), and I will get consistent UI across the board. On the other hand with the |
The purpose of the Like adding a custom background/border or an icon before it.
If you force it in |
@justinmc I think we can proceed with this PR.
It is true that these are separate problems, but for consistency and discoverability it seems better to have |
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.
Thanks for the arguments for keeping this on InputDecorator, I think that all makes sense. Just some small stuff to clean up here.
@@ -440,7 +451,9 @@ class _HelperErrorState extends State<_HelperError> with SingleTickerProviderSta | |||
begin: const Offset(0.0, -0.25), | |||
end: Offset.zero, | |||
).evaluate(_controller.view), | |||
child: widget.error ?? Text( | |||
child: widget.error | |||
?? widget.errorBuilder?.call(widget.errorText!) |
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.
This implies that if errorBuilder is non-null, then errorText must also be non-null. Can you add an assertion for that? And we should probably mention it in the docs for errorBuilder too.
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.
There is already an assert at the top of this function to confirm that either error or errorText is provided.
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.
What I mean is slightly different I think:
assert(errorBuilder == null || errorText != null, 'errorBuilder must be used with errorText')
And I think that should go in InputDecoration's initializers.
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.
Adding it at constructors won't work. errorBuilder will always be provided, and whether or not errorText exists depends on the validator (which is kind of the point of this PR). What we need to guard against is that errorBuilder should not be called without an errorText.
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.
Ah you're right, errorText can be null, and we can't distinguish between null and not passed at all here.
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.
Good discussion here 👍
I will say: even if they can't be moved to a public class, moving some of these asserts to the _HelperError
constructor would make debugging a bit easier, since there would be a shorter stacktrace to follow.
The test you suggested is already added at line 5333 |
Oh sorry for missing this. I feel like we should just keep it since they serve slightly different uses and the assertions should prevent people from using both. Any thoughts @bleroux? |
In that case, no further changes are needed in the PR. Please feel free to merge whenever you'd like. |
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 would be better to add
TextFormField.errorBuilder
and, when not null, use it to override InputDecoration.error instead of InputDecoration.errorText.
I think overall, I agree with what @bleroux said earlier, my preference would be for this to be exclusive to TextFormField
rather than having it on InputDecoration
as well.
If we do keep it on InputDecoration
, would it make sense to also add it to InputDecorationTheme
?
@@ -440,7 +451,9 @@ class _HelperErrorState extends State<_HelperError> with SingleTickerProviderSta | |||
begin: const Offset(0.0, -0.25), | |||
end: Offset.zero, | |||
).evaluate(_controller.view), | |||
child: widget.error ?? Text( | |||
child: widget.error | |||
?? widget.errorBuilder?.call(widget.errorText!) |
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.
Good discussion here 👍
I will say: even if they can't be moved to a public class, moving some of these asserts to the _HelperError
constructor would make debugging a bit easier, since there would be a shorter stacktrace to follow.
Based on this comment and this one, there seems to be a consensus for adding this to both There's a big caveat to this change: passing an inline callback into a Perhaps it doesn't need to be done in this PR, but at some point we should make sure that this info is accessible in our documentation, since I imagine it could trip up a lot of folks. |
Co-authored-by: Nate Wilson <nathan.wilson1232@gmail.com>
Co-authored-by: Nate Wilson <nathan.wilson1232@gmail.com>
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, pending Justin's comment being addressed.
I definitely feel that we should make a concerted effort to improve our documentation with regards to inline callbacks & equality checks, but that can stay separate from this PR.
Thanks for the contribution!
assert( | ||
_hasError, | ||
'errorText or error must be provided' | ||
); |
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.
Thanks for the update!
Attaching asserts to the constructor is nice, but so is utilizing existing getters 👍
@@ -5346,6 +5346,38 @@ void main() { | |||
throwsAssertionError, | |||
); | |||
}); | |||
|
|||
testWidgets('InputDecorator throws when error and errorBuilder are provided', (WidgetTester tester) async { |
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.
+1 to this, it'd be nice to have full test coverage here.
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 @MohiuddinM, apologies for approving and then pulling a switcheroo, but I feel concerned about whether or not landing this change is a good idea.
My main question is: what use case benefits from having errorBuilder
as an InputDecoration field, and how does it compare to our current options?
I am dealing with 40 form fields and using the same InputDecoration (because the UI and theeming for all are the same, only labels are different). This is the same case most apps deal with, where text inputs need to look the same and hence share InputDecoration.
It'd be nice to allow a bunch of widgets to inherit this info from the ambient Theme, though they can also inherit it via class inheritance:
class MyTextField extends TextFormField {
MyTextField({
// ...
}) : super(errorBuilder: _errorBuilder);
static Widget _errorBuilder(String errorText) {
return Text(errorText);
}
}
It could also be structured as a widget that builds a form field, similar to #59059 (comment).
The fact that validators are exclusive to FormField widgets, along with how passing callbacks to a ThemeData can easily lead to performance setbacks for less experienced developers, leads me to believe that this perhaps isn't something we'd want as part of the InputDecoration API.
But I'd be very interested to hear your thoughts!
As we talked about before, there are no options right now because using error and validator together throws an error, and I think it's broken. Also, from what I understand the performance concerns you have are about adding a callback to InputDecorationTheme, but there are already callbacks in themes (see #156275 (comment)). Even then if you decide to keep your concerns then for the InputDecorationTheme we can just take a specialized widget there instead of a callback e.g.
and then a theme takes an instance of this class instead of a callback. With the upcoming macros, this solution will play even nicer, because then we can turn a callback into the above with just an annotation:
|
Thanks @MohiuddinM! The specialized widget sounds very cool—I'm very much looking forward to macros! Though to be consistent with other Flutter APIs I think a callback would be ideal. Apologies if there was a miscommunication: I'm totally fine with the Overall, it seems to me like since |
Consistent with which Flutter API? There are plenty of places in themes where it takes instances of specific classes, instead of callbacks. I also prefer that since it is less likely to have breaking changes this way.
I added it to InputDecoration since it is shared across different types of widgets, and thus provides an easy-to-use, reusable, and consistent way of customizing/styling an error message. Frankly speaking, I do not see any pros of adding it separately to all the widgets, instead of just adding it to InputDecoration. @justinmc Some really good points for future refactoring for InputDecoration will be that:
In this way, third-party packages can implement these changes themselves, and as a result, take some load off of the Flutter team. |
Unique builder callbacks are fairly common, but as of right now I don't believe that any Flutter API asks for a widget with a unique build method.
The main pro, in my opinion, is that complicating the |
Sorry for not making it clear. It's not a Widget, it's a class (just like many others in themes e,g -Decoration, -Style, etc.) that can contain error-related properties. Edit: I updated my comment to make it clear |
Ah, gotcha! I'd be interested in exploring this a bit more, in the context of a larger In the meantime, I'd still be happy to approve adding an |
@MohiuddinM Your problem is that you don't want to specify the errorBuilder in every TextFormField, you want to just specify it in the InputDecoration like this, right? You have lots of TextFormFields all over your application that all use the same InputDecoration it sounds like?
And you also don't want to write a wrapper around TextFormField or write your own TextFormField. |
If there were an errorBuilder on TextFormField and DropDownFormField then ofc I would have just used that instead of arguing about that here. But since there isn't one, so I came up with a solution that maximizes consistency and ease of use. If you don't like my design then feel free to close this PR, and add it the way you would like that would allow people to customize the errorText. |
Alright I'm going to go ahead and close this in preference of a solution based on TextFormField like #157087 in order to maintain the separation of concerns between InputDecorator and TextFormField. @MohiuddinM I really appreciate your work on this PR, thank you for opening it and for putting up with all of our discussion here. We wouldn't have been able to decide how this should be fixed without seeing it done this way first. |
## Description This PR adds the `TextFormField.errorBuilder`property which makes it possible to customize the widget used to display the error message. Implementation based on #156275 (comment) ## Related Issue Fixes [Unable to use validator along with error widget in TextFormField](#133629) Fixes #135292 ## Tests Adds 1 tests.
Fixes #133629
Fixes #135292
Added
errorBuilder
property toInputDecoration
, allowing users to customize the appearance of validation errors shown under the TextFormField.Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.