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

FormBuilderFields Type Alias #1154

Conversation

erayerdin
Copy link
Contributor

@erayerdin erayerdin commented Nov 24, 2022

Solution description

In most cases, the type is inferred when FormBuilderState.fields is assigned to a variable. An example:

// the type is inferred.
final fields = _formKey.currentState?.fields;

However, when the dev needs to pass the reference of fields to a function, closure definition, constructor etc., they have to explicitly define its type. Currently, the type for FormBuilderState.fields is Map<String, FormBuilderFieldState<FormBuilderField<dynamic>, dynamic>>. So, consider the simple function (which might also be taken as an event definition in BLoC) which needs to take fields as input:

void logIn({required Map<String, FormBuilderFieldState<FormBuilderField<dynamic>, dynamic>> fields}) {
  // or Map<String, FormBuilderFieldState<FormBuilderField, dynamic>>
  // assuming fields have `email` and `password` fields...
  // ...which will be used to log in using Firebase
}

So, instead of having this long type definition, we can use typedef to create an alias, which is the primary purpose of this PR. So, new logIn function would be:

void logIn({required FormBuilderFields fields}) {
  // ...
}

Pros:

  • Shorter

Cons (that I can think of):

  • There's also a class named FormBuilderField in the library, might lead to confusion or misuse.

To Do

  • Read contributing guide
  • Check the original issue to confirm it is fully satisfied
  • Add solution description to help guide reviewers
  • If apply, add documentation to code properties and package readme

@codecov
Copy link

codecov bot commented Nov 25, 2022

Codecov Report

Merging #1154 (8200425) into main (c964ef1) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1154   +/-   ##
=======================================
  Coverage   84.93%   84.93%           
=======================================
  Files          19       19           
  Lines         697      697           
=======================================
  Hits          592      592           
  Misses        105      105           
Impacted Files Coverage Δ
lib/src/form_builder.dart 69.76% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@erayerdin
Copy link
Contributor Author

I think the build error stems from something not existing in beta branch. Other PRs as well have this fail.

@deandreamatias
Copy link
Collaborator

Hi @erayerdin
Thanks for your contribution
I will think about the name type and review the PR in next days.
I not in home right now

@deandreamatias deandreamatias merged commit 11e8cba into flutter-form-builder-ecosystem:main Nov 27, 2022
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.

2 participants