feat: Tag Filter component#1087
Conversation
Signed-off-by: Adhitya Mamallan <adhitya.mamallan@uber.com>
|
Show all should be the only selected tag if all is selected |
| startEnhancer?: React.ComponentType<{ theme: Theme }>; | ||
| }; | ||
|
|
||
| export type Props<T extends string> = { |
There was a problem hiding this comment.
What's the purpose of T extends string?
Is it not sufficient to use string instead?
There was a problem hiding this comment.
I wanted to allow usage to specify as strict a type as necessary. In theory, a user could just create a tag filter with T as string, but they could also use something like "WorkflowStatus".
| expect(mockOnChangeValues).toHaveBeenCalledWith(['opt2']); | ||
| }); | ||
|
|
||
| it('calls onChangeValues with all tag keys when clicking "Show all" and no values are selected', async () => { |
There was a problem hiding this comment.
Can you add an overview of user interactions in the Test Plan?
I'm wondering why "all tag keys" are provided when "no values are selected"? I wonder if there's a typo in the latter?
There was a problem hiding this comment.
Essentially, if there are no values selected before the user clicks "Show all", we select all values. I can make the test name clearer.
There was a problem hiding this comment.
I don't think this is the intended behaviour (attaching the design).
There was a problem hiding this comment.
Agreed; I tried out both cases:
- "Show all" visibly selects all the other tags
- "Show all" clears the other tags but sets the query params to select all of them
The first one felt more intuitive to me when I tried it out. We can discuss and reevaluate this if you'd like
There was a problem hiding this comment.
Setting query params with all is something we can work around, i think having ALL value would be better specially for persistence as it keeps working even when we change filter values.
But from UX perspective, we can have a quick discussion on what seems to be better. Asking couple of users may help too.
There was a problem hiding this comment.
The intended behavior is as follows:
- "Show all" is selected by default in both categories, hence showing all event types and statuses
- If the user clicks "Workflow" or any other type, it deselects "Show all" and selects "Workflow". Now it only shows "Workflows" in the list. If the user clicks any other type, it adds it to the filtered list.
- If the user clicks "Show all" again, then it deselects all other options and selects "Show all". This will reset type and show everything.
- The above pattern is the same for "Status": either individual statutes are selected and shown, or "Show all" to reset/show everything.
- For each individual filter that is activated, it will update the count to reflect how many are selected (eg., selecting "Activity" and "Failed" will show "2" active filters)
macrotim
left a comment
macrotim
left a comment
There was a problem hiding this comment.
Selecting "Request Changes" because I mistakenly approved.
Wish I could unapprove instead but this may be the best option.
Signed-off-by: Adhitya Mamallan <adhitya.mamallan@uber.com>
4 participants
Summary
Add TagFilter component, which uses a set of selectable tags as a multi select filter.
Test plan
Unit tests + tested in a follow-up PR to add the filter in the History V2 page (colours not final).