Skip to content
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

Merged
merged 1 commit into from
Jun 13, 2020

Conversation

awhitford
Copy link
Collaborator

  • The errorText was not propagating to the decoration
  • The country code is only prepended when a phone number is specified so that the value is empty when there is no phone number specified
  • Other minor tweaks

@VOIDCRUSHER
Copy link
Contributor

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

@awhitford
Copy link
Collaborator Author

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

@VOIDCRUSHER
Copy link
Contributor

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.

@danvick
Copy link
Collaborator

danvick commented Jun 11, 2020

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.
Since your suggestions will lead to breaking changes, I'd suggest that we merge this in - since it contains fixes for some valid concerns as it were - and create a separate PR for the suggested improvements.

@danvick
Copy link
Collaborator

danvick commented Jun 11, 2020

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

@awhitford
Copy link
Collaborator Author

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

@danvick danvick merged commit f27af78 into flutter-form-builder-ecosystem:master Jun 13, 2020
@awhitford awhitford deleted the phone-val branch June 13, 2020 18:41
@awhitford
Copy link
Collaborator Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants