Multi select changes #696
Multi select changes #696
Conversation
Codecov Report
@@ Coverage Diff @@
## main #696 +/- ##
=======================================
Coverage 85.19% 85.20%
=======================================
Files 782 782
Lines 16060 16048 -12
Branches 2059 2054 -5
=======================================
- Hits 13683 13674 -9
+ Misses 2344 2341 -3
Partials 33 33
Continue to review full report at Codecov.
|
| const searchBar = spectator.query('.search-bar', { root: true }); | ||
| expect(searchBar).toExist(); | ||
|
|
||
| spectator.triggerEventHandler(SearchBoxComponent, 'valueChange', 'fi'); |
There was a problem hiding this comment.
Not sure how these tests worked before. PopoverModule was not added in the shallow test. So the popover content could never get projected.
Also, triggerEventHandler doesn't provide a {root: true} option. Since the search box would be outside the host element's scope, this trigger call just doesn't work.
There was a problem hiding this comment.
Not sure how these tests worked before. PopoverModule was not added in the shallow test.
That's how - it wasn't using the real popover (so those tags are basically ignored), which your test is now. So only the searchbox was mocked as an actual component - but it was never projected. That's also why the root wasn't there before.
There was a problem hiding this comment.
You also might be able to use read: DebugElement to trigger the event handler since you can't use a selector directly in there.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…ce-ui into multi-select-changes
This comment has been minimized.
This comment has been minimized.
| <ht-divider *ngIf="this.showAllOptionControl || this.enableSearch"></ht-divider> | ||
| <div *ngFor="let item of filteredItems" (click)="this.onSelectionChange(item)" class="multi-select-option"> | ||
| <ht-divider *ngIf="this.enableSearch" class="divider"></ht-divider> |
There was a problem hiding this comment.
nit: this could just move inside the enable search ng container
| display="${ButtonStyle.Text}" | ||
| label="Clear Selected" | ||
| (click)="this.onClearSelected()" | ||
| ></ht-button> |
There was a problem hiding this comment.
Do we always want the clear/select all? I can't imagine we would in all places, seems better to allow these buttons to be configurable (as they were)
There was a problem hiding this comment.
+1. We had intentionally left these options configurable in the select component. Even for multi select, the UX for a filter usecase with 2 or 3 items would be better without the clutter of clear all / select all options. So, configurable would be better.
SelectComponent has a sample implementation that we could adapt here as well
hypertrace-ui/projects/components/src/select/select.component.ts has an example of how we made it configurable. We could use the same strategy here.
There was a problem hiding this comment.
Imo it should be enabled implicitly on certain conditions. For example, we would only show search box and the two buttons when number of options are greater than 5. I would think with an input property we would have to make a similar decision at the usage component level.
There was a problem hiding this comment.
Combined it with enableSearch option for now. However, it will still be weird if number of options are only a few. So I would say let's remove this property too and we can just enable it based on # of options.
There was a problem hiding this comment.
I could buy it being implicit, but it still feels to be like it's pre-supposing a specific usage of multiselect. For example, would we want this on places like a table control, where the various selections aren't really related to each other but are more like boolean config options?
There was a problem hiding this comment.
I think we could use it there. IMO if there are more options (no matter the option type), search and control buttons should show up. If there are only a few options to show then we don't need these capabilities.
There was a problem hiding this comment.
I'm not sure I agree, but I'm certainly fine in deferring making it configurable until we have a spot we want to change the behavior.
There was a problem hiding this comment.
So, It is already controlled by the input enableSearch. But I do need to turn it off for < 5 options. I can do it from the usage side too (something like enableSearch="{{options.length > 5}}"). What would be your suggestion?
There was a problem hiding this comment.
we can leave it internal for now. If we need external config of it, let's evaluate once we hit the specific use case.
| public justify: MultiSelectJustify = MultiSelectJustify.Left; | ||
|
|
||
| @Input() | ||
| /** @deprecated */ |
There was a problem hiding this comment.
You removed the actual implementation of this - that's not deprecating, it's breaking. If we're going to break it, might as well remove it (but see other comment about leaving these configurable)
There was a problem hiding this comment.
Yeah, I left it here so that we could talk about it :)
| const searchBar = spectator.query('.search-bar', { root: true }); | ||
| expect(searchBar).toExist(); | ||
|
|
||
| spectator.triggerEventHandler(SearchBoxComponent, 'valueChange', 'fi'); |
There was a problem hiding this comment.
Not sure how these tests worked before. PopoverModule was not added in the shallow test.
That's how - it wasn't using the real popover (so those tags are basically ignored), which your test is now. So only the searchbox was mocked as an actual component - but it was never projected. That's also why the root wasn't there before.
| const searchBar = spectator.query('.search-bar', { root: true }); | ||
| expect(searchBar).toExist(); | ||
|
|
||
| spectator.triggerEventHandler(SearchBoxComponent, 'valueChange', 'fi'); |
There was a problem hiding this comment.
You also might be able to use read: DebugElement to trigger the event handler since you can't use a selector directly in there.
| "commitizen": "^4.2.3", | ||
| "cz-conventional-changelog": "^3.3.0", | ||
| "husky": "^5.1.2", | ||
| "husky": "^4.3.8", |
There was a problem hiding this comment.
is this downgrade intentional?
There was a problem hiding this comment.
Yeah. v5 is not free and requires some additional setup to make it work. So we reverted this change.
| display="${ButtonStyle.Text}" | ||
| label="Clear Selected" | ||
| (click)="this.onClearSelected()" | ||
| ></ht-button> |
There was a problem hiding this comment.
+1. We had intentionally left these options configurable in the select component. Even for multi select, the UX for a filter usecase with 2 or 3 items would be better without the clutter of clear all / select all options. So, configurable would be better.
SelectComponent has a sample implementation that we could adapt here as well
hypertrace-ui/projects/components/src/select/select.component.ts has an example of how we made it configurable. We could use the same strategy here.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| displayMode="${SearchBoxDisplayMode.NoBorder}" | ||
| ></ht-search-box> | ||
| <ng-container *ngIf="this.allOptions$ | async as allOptions"> | ||
| <ng-container *ngIf="allOptions.length > 5"> |
There was a problem hiding this comment.
Hmm so if enable search is disabled, these will never show. If true, it shows when at least 5 options? I thought you were describing that the > 5 rule was the default if enableSearch is unset. Anyway we can go with it and address as use cases pop up.
This comment has been minimized.
This comment has been minimized.
Description
Please include a summary of the change, motivation and context.
filteredoptions were not behaving correctly when select option components were added in an async fashion.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:
Documentation
Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.