-
-
Notifications
You must be signed in to change notification settings - Fork 542
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
Fixed the validation for a Form Builder Phone Field #307
Fixed the validation for a Form Builder Phone Field #307
Conversation
@awhitford : I feel like we would need a few additional improvements to really make this PR successful. I didn't implement this FormBuilder widget so I'm just now inspecting the code and testing it. As I understand, _parsePhone is only called during initState and the other critical function, _invokechange, is only called during initState and _openCountryPickerDialog but not as the user types nor when the user commits their input. I think this could be better improved to follow user conventions. Rather than handle a simple string, I think we need to use a special Phonenumber object that encapsulates, the raw input, the country code used, the parsed E.164 result, and possibly the validated status and the local formatted result if available, basically the full output of Phonenumber.parse. This would give developers added flexibility in handling both the user's raw input and parsed result simultaneously. Also, think we should have this called in such a way to support parsing as we type and accept input, which I think is natural for the user. |
@VOIDCRUSHER I agree that there is room for further improvement. However, this patch simply does the minimum to enable phone validation which is currently completely broken and thus preventing its use. This PR will at least fix it so that rudimentary validation works. |
I understand. Can we have it at least so that it allows for validation/parsing on text input as well? It seems like a big blind spot if that important process only happens when the widget initializes and when the country code is changed. |
I appreciate the good work @awhitford. @VOIDCRUSHER, kindly note that this input was included through a PR by a community member who implemented the field as they saw fit. |
I see the point for this. However, when I was implementing this, the main aim of using the phone_number package was to parse the initialValue and separate the country code from the phone to fit our input. I'm not an expert when it comes to validation and formatting of international phone numbers. When I was researching this, I also found that these general validation libraries may be wrong in some instances, and for these reasons, I decided to just utilize the parsing functionality and leave validation to the user. |
I would prefer to use this Phone Field instead of a Text Field, but the broken validation prevents me. I'm anxious to see this fix merged. I'm open to revisiting this with more improvements, but I prefer to work in small incremental changes. I too would like to see the input parsed while typing so that it is visually separated, consistent with the country, and I really need E164 formats as values. I will need more time to play around with this, but the current version with the validation fix gives me something that will function well enough. "Don't let perfect be the enemy of good." |
Thank you! |
errorText
was not propagating to thedecoration