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

chore: Uses mixed case for native filters headers #15433

Conversation

michael-s-molina
Copy link
Member

@michael-s-molina michael-s-molina commented Jun 28, 2021

SUMMARY

Uses mixed case for native filters headers.
Follow up of #15390.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

123462616-ef4a6780-d5c0-11eb-9b6b-83671b360697

123630488-f903e300-d7eb-11eb-81bd-ba7376b0b7df

@pkdotson

TESTING INSTRUCTIONS

Check screenshots.

ADDITIONAL INFORMATION

  • Has associated issue:
  • 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
  • Changes UI

Copy link
Member

@pkdotson pkdotson left a comment

Choose a reason for hiding this comment

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

looks good to me.

@codecov
Copy link

codecov bot commented Jun 28, 2021

Codecov Report

Merging #15433 (6f738de) into master (0b7e524) will not change coverage.
The diff coverage is n/a.

❗ Current head 6f738de differs from pull request most recent head a7fcf6f. Consider uploading reports for the commit a7fcf6f to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master   #15433   +/-   ##
=======================================
  Coverage   76.99%   76.99%           
=======================================
  Files         975      975           
  Lines       50657    50657           
  Branches     6228     6228           
=======================================
  Hits        39001    39001           
  Misses      11445    11445           
  Partials      211      211           
Flag Coverage Δ
javascript 71.70% <ø> (ø)

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

Impacted Files Coverage Δ
...Filters/FilterBar/FilterControls/FilterControl.tsx 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 0b7e524...a7fcf6f. Read the comment docs.

@michael-s-molina michael-s-molina merged commit e713912 into apache:master Jun 28, 2021
amitmiran137 pushed a commit that referenced this pull request Jul 4, 2021
@junlincc
Copy link
Member

@kicaak Kasia, we need some help on redesigning the cascading filter. a recent change removed the indentation that represents parent and children relationship to preserve horizontal space of the modal. All layers of parent-children filters are stack vertically. Would love to hear your input🙏

cc @kgabryje

@kasiazjc
Copy link
Contributor

kasiazjc commented Jul 23, 2021

@junlincc Sure! I have an initial idea, it's quite hard to do it without using the visual representation of the parent - children relationship, but I hope I will be able to help 🤞 I have a few questions/comments about how it works now too.

Current behaviour
Screen-Recording-2021-07-23-at-16 09 47
I have one parent and one child filter. When I open the popover and select one value in the parent filter not only I am not able to go back to the initial popover state (I need to close it and reopen it), but also on the left the field where before was "parent" filter got switched to child filter which is empty. So I kind of had no idea which filter I was editing. This was super confusing to me, is it the expected behaviour?

Initial exploration
I tried out a few things and for now I am thinking about adding labels with the name of the parent filter (designs and copy are quite rough now, but wanted to share the concept). This would at least a little bit indicate what this filter is related to, also added a little indentation of the children's children filters etc, not sure about it yet.
collapsed
expanded

I added collapsing/expanding of the children. It can be used in the popover, but I thought it could be more intuitive to add it in the left navigation with all of the filters. You can play the prototype here. Here is figma file.
ezgif-4-b2c8afaadfa7

Here is the prototype of the popover. I added a tooltip as I think it makes it more clear what this pill refers to.

filterpopover

Let me know what you think 😌

Improvement
I thought that the filers and filter groups can take a long time to create by the user, so maybe it would be nice to let them set some filters/groups to active/not active when applying them?

Use case: I have four group of filters which are quite extensive. All of them work at the same time well, but for some reason I would like to see results only from two groups and right now to do it, I think I would have to remove other two.

Solution: I set two groups to not active and run the filters with "Apply" button. When it comes to the design - I was thinking about the visual indicator of the active/inactive group or filter. Icon could be placed next to the main parent (single filter, master parent) and that would be enough. This could be for example the icon that now triggers the popover.
image

@michael-s-molina
Copy link
Member Author

@kicaak Thank you for your examples. I like the idea of opening the filter hierarchy in the filter bar. After all, the user is just setting filter values just like he does when interacting with non-hierarchical filters. The modal approach has problems especially when we have the scroll bar enabled and the filter that the user is currently editing disappears from the viewport. I also think that it's way easier for the user to edit some value in the middle of the hierarchy because currently, only one level is present in the filter bar. If we are going this way, I think we need a more clear distinction of the hierarchy group when it's expanded. Maybe a blue background color or a highlight border or other fancy way.

One problem that we need to figure out is about the value that represents the hierarchy when collapsed. Currently, it changes depending on which levels are already filled and this brings the confusion that you mentioned.

@junlincc I think it would be nice to move this discussion to a separate issue also so that we can refer to it when coding.

@kasiazjc
Copy link
Contributor

@michael-s-molina thanks for the feedback! 😌

One problem that we need to figure out is about the value that represents the hierarchy when collapsed. Currently, it changes depending on which levels are already filled and this brings the confusion that you mentioned.

Regarding this one - I think that parent filter should always be displayed when the hierarchy is collapsed, because otherwise it is hard to follow what happens. We can add a label or other indicator so the user knows that there are fields that are empty. cc: @junlincc what do you think?

I think we need a more clear distinction of the hierarchy group when it's expanded. Maybe a blue background color or a highlight border or other fancy way.

Agreed, for now I added a divider, but I think it won't work in the long run. The only reasonable solution here is background colour when hierarchy is expanded, but I would not like to use the blueish preset's colour (even if ligter version) as this one should be for special actions/components. Grey may blend with the dashboard, but I will see what else I can do. Also additional thing would be setting up a rule that only one group can be expanded at a time which would help I think.

cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@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 size/XS v1.3 🚢 1.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants