Skip to content
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

feat(native-filter): limit max tag count for selected filter values #14486

Merged

Conversation

einatbar
Copy link
Contributor

@einatbar einatbar commented May 5, 2021

SUMMARY

show max tag count in select filter
resolves: #12462

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

max_tag_count.mov

TEST PLAN

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@codecov
Copy link

codecov bot commented May 5, 2021

Codecov Report

Merging #14486 (95c86bb) into master (9a96dac) will decrease coverage by 0.33%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #14486      +/-   ##
==========================================
- Coverage   77.63%   77.29%   -0.34%     
==========================================
  Files         962      961       -1     
  Lines       49156    48757     -399     
  Branches     6183     6181       -2     
==========================================
- Hits        38161    37688     -473     
- Misses      10792    10867      +75     
+ Partials      203      202       -1     
Flag Coverage Δ
javascript 72.44% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...c/filters/components/Select/SelectFilterPlugin.tsx 64.86% <ø> (-4.37%) ⬇️
superset/utils/network.py 0.00% <0.00%> (-25.81%) ⬇️
superset/connectors/druid/views.py 69.79% <0.00%> (-10.08%) ⬇️
superset/utils/screenshots.py 38.59% <0.00%> (-9.65%) ⬇️
superset/views/tags.py 40.74% <0.00%> (-9.26%) ⬇️
superset/app.py 81.37% <0.00%> (-6.31%) ⬇️
superset/views/schedules.py 59.68% <0.00%> (-4.81%) ⬇️
superset/tasks/async_queries.py 92.30% <0.00%> (-4.71%) ⬇️
superset/dashboards/api.py 87.67% <0.00%> (-4.64%) ⬇️
... and 52 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 9a96dac...95c86bb. Read the comment docs.

@junlincc
Copy link
Member

junlincc commented May 5, 2021

thanks for the PR! I personally love the solution. @amitmiran137
However, we want to offer the transparency to show dashboard consumer what filter values are selected at a glance.
The problem is that the list could get long and take up lots of space. My proposed solution is to offer the option to collapse/minimize the select box, to 2 lines maybe?

@graceguo-supercat @mihir174 @ktmud wdyt?

@junlincc junlincc added hold! On hold dashboard:native-filters Related to the native filters of the Dashboard labels May 5, 2021
@amitmiran137
Copy link
Member

The ability here is to limit the amount of items that are visible.
Currently @einatbar used 5 items and then the rest goes into the count

We can increase that number to what ever feels right
And also the select box can always be opened to see the full list

@amitmiran137
Copy link
Member

We used the out of the box feature bin antD
No collapse/expand ability in the select

@graceguo-supercat
Copy link

graceguo-supercat commented May 5, 2021

@einatbar thanks for the PR. If the filter is growing taller as user typing in more values, what will happen for the filter below this one? In your example, if "Name" is on top of "Gender" filter, will it keep push "Gender" filter down?

@einatbar
Copy link
Contributor Author

einatbar commented May 6, 2021

@graceguo-supercat It will push "Gender" down until the user reaches the max visible tags count. If we set a low max tags count like 5, it will be pushed by 2 rows down at most, depending on values lengths I guess.

@michael-s-molina
Copy link
Member

I also really like the solution. I think that when we have a lot of selected values it's difficult for the user to actually see what's selected. Generally, the user wants to know if a particular value is selected, and for that case opening the select and searching for it can be faster than trying to find it visually in the middle of 30 selected items. Another thing that bugs me is the layout flick of every item below the select.

Independently of which solution we choose, we need to keep the consistency of all selects. For example, in Explore, the selects are expanding so if we choose to use the tag count here, we should also apply the same pattern there.

@junlincc
Copy link
Member

junlincc commented May 7, 2021

  1. when user click "+10...." to view all, we should only show the selected values in the dropdown, instead of scrolling to look for all that have been selected
  2. apply the full solution to Explore select

@michael-s-molina
Copy link
Member

michael-s-molina commented May 7, 2021

when user click "+10...." to view all, we should only show the selected values in the dropdown, instead of scrolling to look for all that have been selected

@junlincc This has an unintended side-effect. If the user is looking to see if a value is selected and it's not, then he can't select it because it won't show.

@junlincc
Copy link
Member

junlincc commented May 7, 2021

@michael-s-molina is the purpose of expanding to view all selected values or to edit select? User can certainly click the empty space to edit the select

@ktmud
Copy link
Member

ktmud commented May 7, 2021

It is possible to add a tooltip to the "+4..." pill to display all remaining items?

@ktmud ktmud changed the title feat: show max tag count in select filter feat(native-filter): limit max tag count for selected filter values May 7, 2021
@michael-s-molina
Copy link
Member

@michael-s-molina is the purpose of expanding to view all selected values or to edit select? User can certainly click the empty space to edit the select

Both. The AntD team decided to use this default behavior because it's important to the user to view which values are selected and also which ones are not, and to quickly be able to select/unselect a value. Most of the time the user is looking to see if a specific value is selected, and not to read through all the values. Personally, I would stick to the default behavior proposed by AntD.

@michael-s-molina
Copy link
Member

michael-s-molina commented May 7, 2021

One thing that we can do is sort the values presenting the selected ones first. Maybe with a separator.

@michael-s-molina
Copy link
Member

It is possible to add a tooltip to the "+4..." pill to display all remaining items?

@ktmud I liked the idea. If we have few values it's easy to check the remaining items. If we have a bunch of values the user can click on the select and search for a specific value.

@rusackas
Copy link
Member

rusackas commented May 7, 2021

It is possible to add a tooltip to the "+4..." pill to display all remaining items?

@ktmud I liked the idea. If we have few values it's easy to check the remaining items. If we have a bunch of values the user can click on the select and search for a specific value.

It seems that where this gets difficult is not when we're in a +4 situation, but when we're in a +104 situation.

For most use cases, I'd bet the select menu isn't absurdly long, and the list of checked/selected items isn't absurdly long either. In that case, clicking the +𝒙 and popping open the select menu to view/add/remove items from the selection seems perfectly acceptable.

However, with LOTS of selections from LOTS of options (let's say, cities in the US... and you've selected 90 of them) then we have a bigger problem. For that I see multiple potential approaches:

  1. When you click the +𝒙 it opens the select menu, with two new features:
    • Selected items at the top
    • A search input to find a specific option to (de)select
    • A tooltip on the +𝒙 to show a lightweight list of items contained therein

  2. Leave the select menu as-is (maybe add the search anyway), but add an interactive popover to the +𝒙 item.
    • Popover would contain the remaining list of selected values.
    • Each value in that list would have a little 🅧 button next to it to remove the selection, just like the other tag/label selections that precede the +𝒙

  3. Both?

Not sure if this makes sense - happy to draw up wireframes if it helps explain (perhaps with @mihir174)

@rusackas
Copy link
Member

rusackas commented May 7, 2021

I think one thing important to mention (although perhaps tangential to the UI discussion) is that we get to the point where we have one select component we use everywhere in Superset, with whatever props/interfaces/styling we need to achieve all these goals. I suspect the AntD component will suffice and we can replace react-select component. Anyway, back to the topic at hand...

@villebro
Copy link
Member

villebro commented May 7, 2021

I personally LOVE this solution, and at the risk of this hiding something vital, I think it solves the much bigger problem of killing the UX by eating the whole filter tab when having lots of selected values.

@mistercrunch
Copy link
Member

Love this improvement, one extra thing that could help is to make sure that the selected values shows on top as you re-open the dropdown as per GitHub

Screen Shot 2021-05-07 at 12 05 30 PM

@mihir174
Copy link
Contributor

mihir174 commented May 7, 2021

I think this is an awesome solution, and that Max's list ordering suggestion improves it even further. An interactive popover on the +x doesn't reveal any more information than the list itself, so I don't think we need one. We could use this for all select components across the product.

i talked with @michael-s-molina , he said he will do this change in a separate PR so that the current PR is not blocked

# Conflicts:
#	superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx
@amitmiran137
Copy link
Member

are there any mixed feelings here on merging this and do follow-ups PR's?

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's merge and refine in subsequent PRs

@villebro
Copy link
Member

Love this improvement, one extra thing that could help is to make sure that the selected values shows on top as you re-open the dropdown as per GitHub

Screen Shot 2021-05-07 at 12 05 30 PM

I opened this PR to implement reordering of the selected values on reopening the dropdown: #14842

cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
Co-authored-by: einatnielsen <einat.bertenthal@nielsen.com>
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
Co-authored-by: einatnielsen <einat.bertenthal@nielsen.com>
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
Co-authored-by: einatnielsen <einat.bertenthal@nielsen.com>
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.3.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels dashboard:native-filters Related to the native filters of the Dashboard hold! On hold size/XS 🚢 1.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[dashboard][native-filter]make single filter select field collapsible