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

fix: add deep compare to update dropdown items #1272

Merged
merged 4 commits into from
Jul 22, 2023

Conversation

deandreamatias
Copy link
Collaborator

@deandreamatias deandreamatias commented Jul 15, 2023

Connection with issue(s)

Close #1252

Connected to #687
Connected to #1239

Solution description

Add a deep compare to reset dropdown only when items are really different.

I considered remove the verification and transfer it responsibility to reset dropdown to developer users (like @danvick says on this comment), but I thinks that this behavior is a important to have embedded on package because is a feature that able implement related fields with minor effort.

Screenshots or Videos

Test behavior of original issue (#1252)

Screencast.from.15-07-23.14.23.59.webm

Test behavior of related issue (#687)

Screencast.from.15-07-23.14.25.01.webm

To Do

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

@deandreamatias deandreamatias self-assigned this Jul 15, 2023
@codecov
Copy link

codecov bot commented Jul 15, 2023

Codecov Report

Merging #1272 (3487bfb) into main (7541ae5) will decrease coverage by 0.05%.
The diff coverage is 42.10%.

@@            Coverage Diff             @@
##             main    #1272      +/-   ##
==========================================
- Coverage   84.98%   84.94%   -0.05%     
==========================================
  Files          20       21       +1     
  Lines         706      724      +18     
==========================================
+ Hits          600      615      +15     
- Misses        106      109       +3     
Impacted Files Coverage Δ
lib/src/extensions/generic_validator.dart 0.00% <0.00%> (ø)
lib/src/fields/form_builder_dropdown.dart 83.33% <61.53%> (+5.55%) ⬆️

... and 1 file with indirect coverage changes

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

Copy link
Collaborator

@danvick danvick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @deandreamatias, please check the suggested changes.

lib/src/fields/form_builder_dropdown.dart Outdated Show resolved Hide resolved
lib/src/fields/form_builder_dropdown.dart Outdated Show resolved Hide resolved
@danvick
Copy link
Collaborator

danvick commented Jul 19, 2023

Other than the highlighted error, I think the code is well done.

HOWEVER, there is still a loophole to this condition currentlyValues.contains(initialValue).
As I had indicated in point number 2 of my original comment, if the List contains objects of a class that doesn't override the equality operator (==), the condition will still return false.

Example:

main(){
  print([
    Continent(id: 1, name: 'Africa'), 
    Continent(id: 2, name: 'Asia'), 
    Continent(id: 3, name: 'Europe'),
  ].contains(Continent(id: 1, name: 'Africa'))); // PRINTS `true` because the Continent class is 'equatable'

  // WHILE

  print([
    Country(id: 2, name: 'India', continentId: 2),
    Country(id: 3, name: 'Spain', continentId: 3),
    Country(id: 3, name: 'France', continentId: 3),
  ].contains(Country(id: 2, name: 'India', continentId: 2))); // PRINTS `false` because the Country class is NOT 'equatable'

}
class Continent {
  final int id;
  final String name;

  Continent({required this.id, required this.name});

  @override
  operator ==(Object other){
    return other is Continent && other.id == id;
  }

  @override
  int get hashCode => id.hashCode;
}

class Country {
  final int id;
  final String name;
  final int continentId;

  Country({required this.id, required this.name, required this.continentId});
}

It is therefore up to the user to make sure that that's the case.

For example, in the example of the original issue that we're solving, if the Line class was "equatable", the issue wouldn't have occurred in the first place.

Since there's nothing more we could do about that, I applaud @deandreamatias's efforts to prevent the user of the library from shooting themselves in the foot. Good work!

You can resolve the reviews then go ahead and merge. Thanks.

@deandreamatias deandreamatias merged commit 8d17143 into main Jul 22, 2023
3 of 5 checks passed
@deandreamatias deandreamatias deleted the fix-1252-dropdown-reset branch July 22, 2023 13:41
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.

[FormBuilderDropdown]: <If onChanged Function is added, the FormBuilderDropdown stop working>
2 participants