feat: adding server side search support

[anandtiwary]

Description

Please include a summary of the change, motivation and context.

Testing

Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.

Checklist:

• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• Any dependent changes have been merged and published in downstream modules

Documentation

Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.

[anandtiwary]

Need feedback on the api.

Goal:
Support server side search through the multi select.

Questions:

• Naming.

• How should we combine enable client Search and enable server search? We need both

• Should we show no data message in case options list is empty
  

• Should we show search box when number of options are less (example < 5). IMO it seems unnecessary.

[codecov]

Codecov Report

Merging #723 (065255e) into main (17dddb5) will decrease coverage by 0.00%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##             main     #723      +/-   ##
==========================================
- Coverage   85.15%   85.15%   -0.01%     
==========================================
  Files         784      784              
  Lines       16064    16070       +6     
  Branches     1997     1999       +2     
==========================================
+ Hits        13680    13685       +5     
- Misses       2351     2352       +1     
  Partials       33       33              

Impacted Files	Coverage Δ
...ponents/src/multi-select/multi-select.component.ts	98.43% <85.71%> (-1.57%)	⬇️
...components/src/multi-select/multi-select.module.ts	100.00% <100.00%> (ø)

---

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 17dddb5...065255e. Read the comment docs.

[aaron-steinfeld]

What if we took a page out of the table api (hah!) and took in some like a searchProvider object instead. We could default it to a client side version, but provide a server implementation in other places.

[anandtiwary]

oh god. let me check

[anandtiwary]

That sounds complicated. Can we do it with just may be a searchMode enum (Implicit and external)? I think eventually, we would have a separate basic filter multi select component, but we need this as a stepping stone.

[aaron-steinfeld]

OK looked at multiselect - the search provider wouldn't work as well because the data is actually coming in as content children vs an array like some of the other components, so an output is the only way that comes to mind to accomplish this - but let's clean up the ambiguity between the two enable search inputs as you mentioned.

[anandtiwary]

so does this look good?

@Input()
public enableSearch: boolean = false;

@Input()
public searchMode: MultiSelectSearchMode = MultiSelectSearchMode.Implicit;

...


export const enum MultiSelectSearchMode {
  Implicit = 'implicit',
  External = 'external'
}

[aaron-steinfeld]

I'd rather boil down another input and make the enum values more explicit, but same idea:

@Input()
public searchMode: MultiSelectSearchMode = MultiSelectSearchMode.Disabled;
@Output
public searchValueChange: EventEmitter<string> = new EventEmitter<string>();

...

export const enum MultiSelectSearchMode {
  Disabled = 'disabled', // Search is not available
  CaseInsensitive = 'case-insensitive', // Current available values are filtered in a case insensitive way and an emit is triggered
  EmitOnly = 'emit-only' // Current available values not filtered, but an emit still triggered
}

[anandtiwary]

Cool.

[aaron-steinfeld]

this case should still emit. There's no harm in doing so - it could be useful for a component's consumer to know when the search has changed. I'd expect that from a lib component and here it's actually more complex to avoid. Also, it says so in the enum comment :)

[anandtiwary]

I missed the comment. I think this can be confusing. Until there is a known use case, I think we should not emit for this mode since filtering is applied internally. That's why i was keeping Implicit as it's name.

[anandtiwary]

I would prefer calling it Implicit for now and keep the emit logic as above.

[aaron-steinfeld]

What part do you find confusing? A component is expected to emit when its state changes - the caller decides whether they care or not.

The name implicit really doesn't indicate anything to the caller - it just says that something is not directly expressed. But since we're directly expressing that, why not have it describe what it's actually doing?

[anandtiwary]

If search is happening implicitly then why emit on the search value. As a consumer am I supposed to act on it? The name case-insensitive doesn't convey well that there is a auto filtering applied

[anandtiwary]

I am fine with that name, but with that name I don't think we should emit. Can we keep the same name but add the emit logic after we have a valid use case?

[aaron-steinfeld]

The reason I'm caught up on this is that it's not that we're omitting something until we need it - that, I'm pretty much always for. Here, we're adding extra logic to make the behavior of an output inconsistent until we need to use it. In that case, I'd much rather have consistency even if we're not always using it. Outputs are meant to tell you about a state change, in this case, it's telling us that the search string has changed - the host is the one deciding whether to do anything with that or not.

[anandtiwary]

oh come on :)

[anandtiwary]

updated

[aaron-steinfeld]

you're my hero!

[aaron-steinfeld]

this condition is non obvious to me - the two search modes don't really dictate when the search box should or should be shown - I'd think it would be either the same logic for both, or no logic and let the caller dictate by explicitly disabling filter if they're not providing 5+ options (can't remember where exactly that conversation landed in the previous PR).

[anandtiwary]

That problem comes up when you search and get less than 5 results. You are stuck at that point. We can't do it with an option, since this logic will not work with server side search.

[aaron-steinfeld]

That's actually why I'd rather take the logic out entirely - if I express a search mode, show it. If I want to hide the search box in certain conditions, that's trivially done on the caller side.

[anandtiwary]

The only thing is, for consistent behavior we will have to put searchMode= options.length > 5 ? MultiSelectSearchMode.Implicit : MultiSelectSearchMode.Disabled in every usage. It looks really weird to show search with just 1 option.

[aaron-steinfeld]

There's very places where we have a variable number of options, anyway - and I think with any static small number (such as 6) search doesn't really make sense.

[anandtiwary]

We need the options.length > 1 logic for displaying buttons 'select all' and 'clear selected' buttons. I can remove allOptions.length > 5 conditions if you feel it is not useful and condition is non obvious.

[anandtiwary]

Removed

[github-actions]

Unit Test Results

    4 files  ±0  245 suites  ±0   14m 28s ⏱️ -49s
879 tests +1  879 ✔️ +1  0 💤 ±0  0 ❌ ±0 
883 runs  +1  883 ✔️ +1  0 💤 ±0  0 ❌ ±0 

Results for commit 624009a. ± Comparison against base commit 17dddb5.