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

Minimize code duplication #59

Merged
merged 3 commits into from
Oct 11, 2021
Merged

Minimize code duplication #59

merged 3 commits into from
Oct 11, 2021

Conversation

ajilo297
Copy link

@ajilo297 ajilo297 commented Oct 10, 2021

I am working on another PR which would allow to remove country flags from the Selector list. This PR is just to suggest some improvements in the code before I do that. I have attempted to do 2 things in this PR

  1. Replace Function(Country) onCountrySelected with ValueChanged<Country> onCountrySelected.
  2. Use CountrySelectorNavigator class to hold the common values and return a CountrySelector with appropriate values

Let me know how this looks.

@cedvdb
Copy link
Owner

cedvdb commented Oct 11, 2021

Nice PR, thanks.

In the same vein as your other changes, maybe the defaults values should be the same for all and set up on super (specifically the three boolean values).

However the way defaults work in dart might make the code less readable. What do you think? I can merge this as is though.

@ajilo297
Copy link
Author

Nice PR, thanks.

In the same vein as your other changes, maybe the defaults values should be the same for all and set up on super (specifically the three boolean values).

However the way defaults work in dart might make the code less readable. What do you think? I can merge this as is though.

@cedvdb So you mean something like super(addSeparator: addSeparator ?? true) and make the booleans required in CountrySelectorNavigator?

@cedvdb
Copy link
Owner

cedvdb commented Oct 11, 2021

I meant that the defaults are repeated in every constructors but I don't see how we could resolve that without a bunch of null checks:

    bool addSeparator = true,
    bool showCountryCode = true,
    bool sortCountries = false,

but I don't see how we can cleanly remove that without a bunch of null checks, this might be a bit less readable:

  const CountrySelectorNavigator({
   this.countries,
   this.favorites,
   bool? addSeparator,
   bool? showCountryCode,
   bool? sortCountries,
   this.noResultMessage,
 }):
   addSeparator = addSeparator ?? false,
   showCountryCode = showCountryCode ?? true,
   sortCountries = sortCountries ?? true;
   
   
 const DialogNavigator({
   List<Country>? countries,
   List<String>? favorites,
   bool? addSeparator,
   bool? showCountryCode,
   bool? sortCountries,
   String? noResultMessage,
 }) : super(
         countries: countries,
         favorites: favorites,
         addSeparator: addSeparator,
         showCountryCode: showCountryCode,
         sortCountries: sortCountries,
         noResultMessage: noResultMessage,
       );

I do believe the defaults should be the same for all of them though, and that is a way to enforce it but that's minor.

@cedvdb cedvdb added the 4.0.1 label Oct 11, 2021
@cedvdb cedvdb merged commit 8b2f870 into cedvdb:dev Oct 11, 2021
@cedvdb
Copy link
Owner

cedvdb commented Oct 11, 2021

My hope is dart-lang/language#1855 could alleviate this anyway.

cedvdb added a commit that referenced this pull request Oct 15, 2021
* Minimize code duplication (#59)

* Added common params to abstract class

* Changed onCountrySelected to a ValueChanged object

* Simplified code

* phone_numbers_parser v4.0.1 (#60)

* fix 54 (#63)

* fix 54

* changelog

Co-authored-by: Ajil Oommen <ajilo297@gmail.com>
cedvdb added a commit that referenced this pull request Oct 15, 2021
* Minimize code duplication (#59)

* Added common params to abstract class

* Changed onCountrySelected to a ValueChanged object

* Simplified code

* phone_numbers_parser v4.0.1 (#60)

* fix 54 (#63)

* fix 54

* changelog

* update version

Co-authored-by: Ajil Oommen <ajilo297@gmail.com>
cedvdb added a commit that referenced this pull request Oct 16, 2021
* Minimize code duplication (#59)

* Added common params to abstract class

* Changed onCountrySelected to a ValueChanged object

* Simplified code

* phone_numbers_parser v4.0.1 (#60)

* fix 54 (#63)

* fix 54

* changelog

* update version

* Rename invalidX to validX (#67)

* rename validity

* readme

Co-authored-by: Ajil Oommen <ajilo297@gmail.com>
cedvdb added a commit that referenced this pull request Oct 18, 2021
* Minimize code duplication (#59)

* Added common params to abstract class

* Changed onCountrySelected to a ValueChanged object

* Simplified code

* phone_numbers_parser v4.0.1 (#60)

* fix 54 (#63)

* fix 54

* changelog

* update version

* Rename invalidX to validX (#67)

* rename validity

* readme

* Issue 69 params passthrough (#70)

* passthrough

* changelog

* autovalidate mode

* remove unnecessary print statement

* Controller select all (#72)

* passthrough

* changelog

* autovalidate mode

* remove unnecessary print statement

* ff

* controllers refactor

* controller

* changelog

* changelog

Co-authored-by: Ajil Oommen <ajilo297@gmail.com>
cedvdb added a commit that referenced this pull request Oct 18, 2021
* Minimize code duplication (#59)

* Added common params to abstract class

* Changed onCountrySelected to a ValueChanged object

* Simplified code

* phone_numbers_parser v4.0.1 (#60)

* fix 54 (#63)

* fix 54

* changelog

* update version

* Rename invalidX to validX (#67)

* rename validity

* readme

* Issue 69 params passthrough (#70)

* passthrough

* changelog

* autovalidate mode

* remove unnecessary print statement

* Controller select all (#72)

* passthrough

* changelog

* autovalidate mode

* remove unnecessary print statement

* ff

* controllers refactor

* controller

* changelog

* changelog

* new doc

Co-authored-by: Ajil Oommen <ajilo297@gmail.com>
cedvdb added a commit that referenced this pull request Nov 7, 2021
* 4.3.0 (#73)

* Minimize code duplication (#59)

* Added common params to abstract class

* Changed onCountrySelected to a ValueChanged object

* Simplified code

* phone_numbers_parser v4.0.1 (#60)

* fix 54 (#63)

* fix 54

* changelog

* update version

* Rename invalidX to validX (#67)

* rename validity

* readme

* Issue 69 params passthrough (#70)

* passthrough

* changelog

* autovalidate mode

* remove unnecessary print statement

* Controller select all (#72)

* passthrough

* changelog

* autovalidate mode

* remove unnecessary print statement

* ff

* controllers refactor

* controller

* changelog

* changelog

Co-authored-by: Ajil Oommen <ajilo297@gmail.com>

* new demo  (#74)

* Minimize code duplication (#59)

* Added common params to abstract class

* Changed onCountrySelected to a ValueChanged object

* Simplified code

* phone_numbers_parser v4.0.1 (#60)

* fix 54 (#63)

* fix 54

* changelog

* update version

* Rename invalidX to validX (#67)

* rename validity

* readme

* Issue 69 params passthrough (#70)

* passthrough

* changelog

* autovalidate mode

* remove unnecessary print statement

* Controller select all (#72)

* passthrough

* changelog

* autovalidate mode

* remove unnecessary print statement

* ff

* controllers refactor

* controller

* changelog

* changelog

* new doc

Co-authored-by: Ajil Oommen <ajilo297@gmail.com>

* fix focus issue (#76) (#77)

* fix focus issue

* fixes auto focus

* Add SV language

Co-authored-by: cedvdb <cedvandenbosch@gmail.com>
Co-authored-by: Ajil Oommen <ajilo297@gmail.com>
cedvdb added a commit that referenced this pull request Nov 7, 2021
* fix imports

* Add swedish language (#78)

* 4.3.0 (#73)

* Minimize code duplication (#59)

* Added common params to abstract class

* Changed onCountrySelected to a ValueChanged object

* Simplified code

* phone_numbers_parser v4.0.1 (#60)

* fix 54 (#63)

* fix 54

* changelog

* update version

* Rename invalidX to validX (#67)

* rename validity

* readme

* Issue 69 params passthrough (#70)

* passthrough

* changelog

* autovalidate mode

* remove unnecessary print statement

* Controller select all (#72)

* passthrough

* changelog

* autovalidate mode

* remove unnecessary print statement

* ff

* controllers refactor

* controller

* changelog

* changelog

Co-authored-by: Ajil Oommen <ajilo297@gmail.com>

* new demo  (#74)

* Minimize code duplication (#59)

* Added common params to abstract class

* Changed onCountrySelected to a ValueChanged object

* Simplified code

* phone_numbers_parser v4.0.1 (#60)

* fix 54 (#63)

* fix 54

* changelog

* update version

* Rename invalidX to validX (#67)

* rename validity

* readme

* Issue 69 params passthrough (#70)

* passthrough

* changelog

* autovalidate mode

* remove unnecessary print statement

* Controller select all (#72)

* passthrough

* changelog

* autovalidate mode

* remove unnecessary print statement

* ff

* controllers refactor

* controller

* changelog

* changelog

* new doc

Co-authored-by: Ajil Oommen <ajilo297@gmail.com>

* fix focus issue (#76) (#77)

* fix focus issue

* fixes auto focus

* Add SV language

Co-authored-by: cedvdb <cedvandenbosch@gmail.com>
Co-authored-by: Ajil Oommen <ajilo297@gmail.com>

* v4.4.0 (#79)

Co-authored-by: Xavier H <xavier.hainaux@gmail.com>
Co-authored-by: Ajil Oommen <ajilo297@gmail.com>
cedvdb added a commit that referenced this pull request Jan 7, 2022
* fix imports

* Add swedish language (#78)

* 4.3.0 (#73)

* Minimize code duplication (#59)

* Added common params to abstract class

* Changed onCountrySelected to a ValueChanged object

* Simplified code

* phone_numbers_parser v4.0.1 (#60)

* fix 54 (#63)

* fix 54

* changelog

* update version

* Rename invalidX to validX (#67)

* rename validity

* readme

* Issue 69 params passthrough (#70)

* passthrough

* changelog

* autovalidate mode

* remove unnecessary print statement

* Controller select all (#72)

* passthrough

* changelog

* autovalidate mode

* remove unnecessary print statement

* ff

* controllers refactor

* controller

* changelog

* changelog

Co-authored-by: Ajil Oommen <ajilo297@gmail.com>

* new demo  (#74)

* Minimize code duplication (#59)

* Added common params to abstract class

* Changed onCountrySelected to a ValueChanged object

* Simplified code

* phone_numbers_parser v4.0.1 (#60)

* fix 54 (#63)

* fix 54

* changelog

* update version

* Rename invalidX to validX (#67)

* rename validity

* readme

* Issue 69 params passthrough (#70)

* passthrough

* changelog

* autovalidate mode

* remove unnecessary print statement

* Controller select all (#72)

* passthrough

* changelog

* autovalidate mode

* remove unnecessary print statement

* ff

* controllers refactor

* controller

* changelog

* changelog

* new doc

Co-authored-by: Ajil Oommen <ajilo297@gmail.com>

* fix focus issue (#76) (#77)

* fix focus issue

* fixes auto focus

* Add SV language

Co-authored-by: cedvdb <cedvandenbosch@gmail.com>
Co-authored-by: Ajil Oommen <ajilo297@gmail.com>

* v4.4.0 (#79)

* add turkish language support (#82)

* 4.4.0

* all tests passing

Co-authored-by: Xavier H <xavier.hainaux@gmail.com>
Co-authored-by: Ajil Oommen <ajilo297@gmail.com>
Co-authored-by: Mücahid Kamber <38380519+thlstfs@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants