-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🐛 Use separate chip groups for tag categories #1903
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1903 +/- ##
==========================================
+ Coverage 39.20% 41.96% +2.76%
==========================================
Files 146 163 +17
Lines 4857 5245 +388
Branches 1164 1336 +172
==========================================
+ Hits 1904 2201 +297
+ Misses 2939 2925 -14
- Partials 14 119 +105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@ibolton336 @rszwajko this looks really good |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few questions:
-
Should the text filter apply to the tag category? What should be shown in the list if you type part of the category name "3rd"? (This kinda assumes the "3rd party" tag category is available on a fresh install)
-
When navigating the drop down with keyboard input, should a page up/down bring checkbox focus with it the same way as up/down does? Right now if I hit page down, the checkbox focus does not follow and I need to use the arrow keys to scroll back to what I was looking at.
Visually looks great!
The chip groups look good and work great.
I'll take one more pass at the code later today.
client/src/app/components/FilterToolbar/MultiselectFilterControl.tsx
Outdated
Show resolved
Hide resolved
IMHO yes.
All tags in that category. Note I haven't refactor the value format that is used internally for searching - it's still
There are 2 cases:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few style comments, but the main one is with the filteredOptions
handling. That needs to be group aware so item filtering works as expected (i.e. filter is applied to both the tag category and tag name)
client/src/app/components/FilterToolbar/MultiselectFilterControl.tsx
Outdated
Show resolved
Hide resolved
client/src/app/components/FilterToolbar/MultiselectFilterControl.tsx
Outdated
Show resolved
Hide resolved
client/src/app/pages/applications/applications-table/applications-table.tsx
Show resolved
Hide resolved
client/src/app/components/FilterToolbar/MultiselectFilterControl.tsx
Outdated
Show resolved
Hide resolved
Key points: 1. drop ununsed feature - using dictionary type for selectedOptions 2. drop state that can be calculated: a) active item from focusedItemIndex b) selectedOptions from filters 3. centralize id calculations - use prefix based on category title Signed-off-by: Radoslaw Szwajkowski <[email protected]>
Signed-off-by: Radoslaw Szwajkowski <[email protected]>
Additional changes: fix problem with chip groups disappearing when other filter type was chosen. Signed-off-by: Radoslaw Szwajkowski <[email protected]>
Signed-off-by: Radoslaw Szwajkowski <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build of filteredOptions
looks great!
LGTM
Addresses issue #1944 introduced by #1903 (follow-up to #1946) The MultiselectFilterControl component discovers groups from provided data. Each group results in a new ToolbarFilter/ChipGroup pair. Before this fix, the keys were based only on group names. In case of Language filter it created a collision with Language group derived from Tags. After this fix, the keys are built by concatenating category title and group name. The category title is known at compile time and is expected to be unique among all filters on the screen. Reference-Url: #1944 Reference-Url: #1903 Reference-Url: #1946 Signed-off-by: Radoslaw Szwajkowski <[email protected]>
Use one ToolbarFilter per options group. First filter provides
the option list. Remaining filters are hidden and used only for
side effects (separate chip groups). The approach follows
similar widgets used by Forklift plugin and OpenShift Console but
uses a flat list of toolbars (nesting trick requires the widget to be
visible all the time).
Related refactorings in MuliselectFilterControl:
a) active item from focusedItemIndex
b) selectedOptions from filters
Resolves: #1774
Reference-Url: kubev2v/forklift-console-plugin#90
Reference-Url: https://github.com/openshift/console/blob/5ba18580676a25e4304df78253aad6a9832d4d56/frontend/public/components/filter-toolbar.tsx#L299