fix(Dropdown): comparison of custom objects

[lucivpav]

Resolves #1786

[codecov]

Codecov Report

Merging #1943 into master will increase coverage by <.01%.
The diff coverage is 83.33%.

@@            Coverage Diff            @@
##           master   #1943      +/-   ##
=========================================
+ Coverage    70.3%   70.3%   +<.01%     
=========================================
  Files         893     893              
  Lines        7890    7894       +4     
  Branches     2309    2311       +2     
=========================================
+ Hits         5547    5550       +3     
- Misses       2330    2331       +1     
  Partials       13      13

Impacted Files	Coverage Δ
...ackages/react/src/components/Dropdown/Dropdown.tsx	87.75% <83.33%> (-0.13%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68f4134...94be010. Read the comment docs.

[layershifter]

LGTM. @silviuavram do you have any comments?

[silviuaavram]

let me take a look because at first glance it's not looking ok.

Edit: will leave original comment but maybe I am missing something.

I don't like it. itemToString should not be used for object compare.

If it's item => item.firstName and we have both Bruce Wayne and Bruce Banner then we are removing both.

Also if you return item.id from it (just to illustrate how you can use it to compare objects), you are breaking the a11y status message: 1234 has been selected, the highlight by character keys and adding the string equivalent in the searchInput on selection. In single search you will select Bruce Wayne and in the input it will appear the id of that object.

We need another way.

[lucivpav]

So I will add a new prop, called compareBy and it will be a function taking an item and returning any.

[silviuaavram]

nice!

[lucivpav]

@layershifter do you agree with the newly created prop?

[lucivpav]

@silviuavram Shift suggest naming the new prop as itemToValue, do you agree?

[silviuaavram]

@lucivpav I am bad at giving names but itemToValue just doesn't seem to illustrate its purpose. If we're using it for comparing items to avoid duplicates, then that has to be reflected in the name.

@jurokapsiar what do you think?

[layershifter]

Why I don't like compareBy()? The name is very similar to Lodash functions:

• compare - points me compare(a, b) where a and b are items
• from compareBy I expect compareBy(a, b, iteratee) where iteratee is a function

https://lodash.com/docs/4.17.15#difference
https://lodash.com/docs/4.17.15#differenceBy

---

ReactSelect uses getOptionLabel and getOptionValue to get text and value for comparisons:

https://github.com/JedWatson/react-select/blob/42f5e8df8dce15a926f580279fb9a6993a3b5de5/packages/react-select/src/builtins.js#L7

https://github.com/JedWatson/react-select/blob/42f5e8df8dce15a926f580279fb9a6993a3b5de5/packages/react-select/src/Select.js#L38

As we have itemToString(), itemToValue() looks like a closest to me.

[silviuaavram]

Then how about we use _.differenceWith that accepts a comparator function, and then we can name it comparator or compare? We achieve same result and the we can have a prop with compare related name, which should make it clear to the user that it's used for comparing objects to remove duplicates.

[jurokapsiar]

From the API point of view, I like that compare or comparator would suggest what the implementation of that function would look like.

However, looking at how it needs to be implemented, it is a function that translates the item to a value:

      const compareBy = (item: ShorthandValue<DropdownItemProps>): string => {
        if (!item || !React.isValidElement(item)) {
          return ''
        }
        return (item as any).id
      }

so calling it itemToValue is better with current implementation.

If you would change the signature to

const compare = (item1, item2) => { ... }

then compare or comparator would make more sense.

[silviuaavram]

So the decision is basically whether updating the implementation and renaming the prop for the sake of clarity is worth it or not. I vote yes, there are cases where users just rely on the prop name for understanding the use, and for me the comparator/compare is clearer.

If not then at least we should make sure that the prop description is obvious related to its purpose.

Up to you @lucivpav .

[lucivpav]

Thank you for your comments. I am settling on calling the prop itemToValue and adding a descriptive comment that makes it clear that it is used when comparing items in multiple selection.