fix(module:select): Null option shows in SelectContent

[anddrzejb]

🤔 This is a ...

• New feature
• Bug fix
• Site / documentation update
• Demo update
• Component style update
• Bundle size optimization
• Performance optimization
• Refactoring
• Code style optimization
• Test Case
• Branch merge
• Other (about what?)

🔗 Related issue link

Fixes #1587
Addresses issues mentioned in this comment.
Addresses proposal in this comment.

💡 Background and solution

To fix the primary issue, in this PR I added a boolean that identifies if there is a SelectOptionItem that has its Value equal to the default value of TItemValue. I use that flag to figure out what to do when the value is selected and when the Select is cleared. On top of that, it also adds the parameter ValueOnClear which will be used as a primary value when Select is cleared. ValueOnClear may be set to a value that does not exist in SelectOptionItem. In that case, the SelectContent is cleared. Otherwise it is set to the existing SelectOptionItem. I included test cases to cover for these scenarios (I think I cover all possibilities, at least until proven otherwise 😏).

During my tests I found out we have some weird issues with Select. It seems it relies a lot on things happening during re-rendering. I am not a fan of this, as it splits responsibilities and is really hard to follow (and thus debug). In my opinion this makes Select more complex & less performant.

📝 Changelog

Language	Changelog
🇺🇸 English	new [Parameter] ValueOnChange - stored Value that will be used when clear button is pressed
🇨🇳 Chinese

☑️ Self Check before Merge

⚠️ Please check all items below before review. ⚠️

• Doc is updated/provided or not needed
• Demo is updated/provided or not needed
• Changelog is provided or not needed

[github-actions]

[anranruye]

Well, I didn't have a look at the codes yet. From your description, I feel this solution makes things more complicated. As we discussed before, the basic logic for Select should be simpler. With no fallback strategy, the Select component accepts all values of TItemValue, if the value is in select-options, then we display that item, otherwise we display nothing or a placeholder identities nothing(null) or a placeholder identities a not-null value which is not in the options. When clear, set the value to default(TItemValue) or ValueOnClear.

With a fallback strategy, when the value is:

1. value in select-options, display that item, no fallback
2. null, display a placeholder, no fallback
3. fallback value defined by the strategy(null, default(TItemValue) or a user specified value), display nothing or a placeholder
4. other values, fallback

That's all. And for most cases, we don't need a fallback strategy, which makes things as simple as I mentioned in the first paragraph.

[anddrzejb]

I am confused about what do you understand when you say "fallback strategy". Do you mean usage of ValueOnClear? If not, could you elaborate and possibly give an example?

I do not think my solution is that complex. What it does handle is possible later inclusion of an item with its value set to default(TItemValue). And also avoids searching every time for an item that would match default(TItemValue). Without it, there would be an unnecessary performance penalty on every clear button pressed.

I do understand you are probably addressing the conversation we had in PR #1906. I read the whole thing again before I posted this PR. To apply all what we established in that discussion would mean a serious refactoring of the Select component. I don't believe we should attempt that before we have a fairly good test coverage for the Select component. And I am not referring to codecov % but to real scenarios. This PR was meant to fix the current issues we have. I think I did apply to clearing what we established in PR #1906. This should be reflected in the test scenarios I created.

[codecov]

Codecov Report

Merging #2023 (5d8dccc) into master (e698e40) will increase coverage by 2.37%.
The diff coverage is 89.87%.

@@            Coverage Diff             @@
##           master    #2023      +/-   ##
==========================================
+ Coverage   27.57%   29.94%   +2.37%     
==========================================
  Files         489      512      +23     
  Lines       32243    24635    -7608     
  Branches        0      238     +238     
==========================================
- Hits         8890     7377    -1513     
+ Misses      23353    17222    -6131     
- Partials        0       36      +36     

Impacted Files	Coverage Δ
components/select/LabelTemplateItem.razor.cs	0.00% <ø> (ø)
components/select/Properties.cs	0.00% <ø> (ø)
components/select/SelectLocale.cs	0.00% <ø> (ø)
...ents/select/internal/DataSourceEqualityComparer.cs	57.14% <ø> (+7.14%)	⬆️
components/select/internal/SelectContent.razor.cs	57.97% <0.00%> (+4.49%)	⬆️
components/select/internal/SelectModeExtensions.cs	100.00% <ø> (ø)
...ponents/select/internal/SelectOptionGroup.razor.cs	0.00% <ø> (ø)
components/select/internal/SelectOptionItem.cs	82.35% <ø> (+2.55%)	⬆️
components/select/internal/ValueLabel.cs	0.00% <ø> (ø)
components/select/SelectBase.cs	71.11% <90.32%> (+14.32%)	⬆️
... and 434 more

---

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 e698e40...5d8dccc. Read the comment docs.

[anranruye]

        internal void RemoveEqualityToNoValue(SelectOptionItem<TItemValue, TItem> option)
        {
            if (TypeDefaultExistsAsSelectOption)
            {
                TypeDefaultExistsAsSelectOption = IsOptionEqualToNoValue(option);
            }
        }

I still think here has a mistake.

[jeffraska]

Hi, I think this PR breaks Two-way binding in Default mode while changing binded value to NULL.

• When I dont have any special NULL value option in DataSource:

1. Select.OnValueChange() with NULL value is called.
2. TItemValue is long? so condition is true and OnInputClearClickAsync() is called.
3. ClearDefaultMode() is called.
4. Value Equals to default(long?) so this method just returns and do nothing, SelectedOptionItems is not changed and user still see previous value.

• When I have special NULL value option in DataSource:

1. Select.OnValueChange() with NULL value is called.
2. EvaluateValueChangedOutsideComponent() is called.
3. ActiveOption is not NULL and Value Equals to default(long?) so this method just returns and do nothing, SelectedOptionItems is not changed and user still see previous value.

Or am I doing something wrong and missing something?

[anranruye]

@jeffraska Yes, I can reproduce this.