feat(native-filters): Re-arrange controls in FilterBar

[kgabryje]

SUMMARY

This PR implements a part of Stage 1 of native filters rework.
Changes included:

• Replace a pencil icon with text button - "+ Add/Edit Filters"
• Move "apply filters" and "clear all" buttons to the bottom of filter bar and make them sticky. The background of the buttons container is semi-transparent (with a gradient) so that scrolling filters looks smoother
• Adjust the text in empty state

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2022-02-17.at.15.04.05.mov

TESTING INSTRUCTIONS

Make sure that every button in Filters Bar works exactly like in the old version

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• 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]

Codecov Report

Merging #18784 (91c5810) into master (57c4d0f) will decrease coverage by 0.00%.
The diff coverage is 68.75%.

@@            Coverage Diff             @@
##           master   #18784      +/-   ##
==========================================
- Coverage   66.32%   66.31%   -0.01%     
==========================================
  Files        1620     1621       +1     
  Lines       63061    63102      +41     
  Branches     6368     6380      +12     
==========================================
+ Hits        41824    41845      +21     
- Misses      19579    19597      +18     
- Partials     1658     1660       +2     

Flag	Coverage Δ
javascript	51.25% <68.75%> (-0.01%)	⬇️

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

Impacted Files	Coverage Δ
...d/components/DashboardBuilder/DashboardBuilder.tsx	72.72% <ø> (-2.57%)	⬇️
...board/components/nativeFilters/FilterBar/index.tsx	66.94% <0.00%> (-2.88%)	⬇️
...ts/nativeFilters/FilterBar/ActionButtons/index.tsx	85.71% <85.71%> (ø)
superset-frontend/src/components/Button/index.tsx	100.00% <100.00%> (ø)
...ilters/FilterBar/FilterControls/FilterControls.tsx	77.77% <100.00%> (+0.63%)	⬆️
...nents/nativeFilters/FilterBar/FilterSets/index.tsx	37.66% <100.00%> (+0.82%)	⬆️
...omponents/nativeFilters/FilterBar/Header/index.tsx	100.00% <100.00%> (+4.76%)	⬆️
superset-frontend/src/dashboard/constants.ts	100.00% <100.00%> (ø)
superset-frontend/src/utils/common.js	89.47% <100.00%> (+0.18%)	⬆️
...nts/controls/DateFilterControl/DateFilterLabel.tsx	40.86% <0.00%> (-10.76%)	⬇️
... and 18 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 57c4d0f...91c5810. Read the comment docs.

[michael-s-molina]

@kgabryje Did you test it with the white background instead of the semi-transparent one? For me, when a filter is passing behind the buttons it looks really weird like the layout broke or something.

I think keeping a white background with the buttons outside of the scrollable area is also consistent with the top buttons.

Screen.Recording.2022-02-17.at.1.49.46.PM.mov

[kgabryje]

@kgabryje Did you test it with the white background instead of the semi-transparent one? For me, when a filter is passing behind the buttons it looks really weird like the layout broke or something.

That change was discussed with @kasiazjc - the purpose is to indicate that there is hidden content under the buttons and user can scroll down.

[michael-s-molina]

That change was discussed with @kasiazjc - the purpose is to indicate that there is hidden content under the buttons and user can scroll down.

But then when there's available content to the top wouldn’t be the same?

[michael-s-molina]

That change was discussed with @kasiazjc - the purpose is to indicate that there is hidden content under the buttons and user can scroll down.

But then when there's available content to the top wouldn’t be the same?

One thing we could do is to keep the scroll always visible when there is one or add some icon like arrows top/bottom or 3 dots to indicate that there is more content available scrolling top or bottom. I prefer to keep the scroll always visible.

[kasiazjc]

That change was discussed with @kasiazjc - the purpose is to indicate that there is hidden content under the buttons and user can scroll down.

But then when there's available content to the top wouldn’t be the same?

One thing we could do is to keep the scroll always visible when there is one or add some icon like arrows top/bottom or 3 dots to indicate that there is more content available scrolling top or bottom. I prefer to keep the scroll always visible.

@michael-s-molina Placement of the buttons at the top and bottom are two separate issues/flows. You scroll from the top to bottom, so when you scroll down you probably remember that there is something at the top. It may not be on the other hand intuitive that you can scroll down especially when the content/field hidden behind the buttons at the bottom is not partially visible.

From the design perspective the way we have scroll bar implemented right now is the intuitive/universal option and looks much cleaner (also that's how it's implemented in browsers etc.). Also adding the gradient behind the bottom section, especially when it comes to the buttons, helps to make the design lighter and more intuitive without it being too blocky - another white block at the bottom, especially when user's screen is smaller, can clutter the page (we already have a lot of different sections, panels etc.). We can change the % of the gradient, but I think this solutions works best here.

You mentioned adding icons as indicators, but it's not as visually easy to understand as the gradient. Additionally we already have a lot of different icons and we found that a lot of times it is confusing to the users what those actually mean.

[michael-s-molina]

@kasiazjc Good point about the different flow. I also don't like additional icons, I just thought that showing the scroll was sufficient to indicate that there are hidden items, and it works both ways. But no problem, it was just an impression that I had while testing. Thanks for the explanation 😉

[kasiazjc]

@michael-s-molina and I appreciate the discussion 🤠 🤝 Thanks!

[jinghua-qa]

/testenv up

[github-actions]

@jinghua-qa Ephemeral environment spinning up at http://34.217.191.177:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

[michael-s-molina]

Code LGTM. @jinghua-qa Can we get QA's approval?

[github-actions]

@kgabryje Ephemeral environment spinning up at http://18.237.112.254:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

[jinghua-qa]

I have 2 questions for the new look:
1, the clear all button is always grey out and make me feel like it is disable, also i feel like the buttons are not consistent, for example the clear button did not have a background but apply all have a grey back ground, and it makes me feel like apply is more standout

2, if i have multiple filters, the scroll bar is hidden, and user need to move the mouse into the filters area to be able to scroll, and there in no clear window to guide the user to move the mouse, which could be a little confusing?

filter.bar.in.visible.mov

[kgabryje]

Thanks for the comments @jinghua-qa! After consulting with @kasiazjc I changed the default color of "Clear all" to a darker one (first screenshot) and changed the color on hover (second screenshot). The light grey color is now only used when button is disabled. As for the apply button standing out - it does stand out by design as it calls a more important action than clear all.

[kasiazjc]

I have 2 questions for the new look: 1, the clear all button is always grey out and make me feel like it is disable, also i feel like the buttons are not consistent, for example the clear button did not have a background but apply all have a grey back ground, and it makes me feel like apply is more standout

2, if i have multiple filters, the scroll bar is hidden, and user need to move the mouse into the filters area to be able to scroll, and there in no clear window to guide the user to move the mouse, which could be a little confusing?

filter.bar.in.visible.mov

Thank you for feedback @jinghua-qa!

1. We want to make the apply all more visible than the clear all, so that's why we have this hierarchy + difference. Plus, it's a standard pattern for this kind of components - usually in filter components "Clear all" action is less visible and shown as a link, so I wouldn't say it's inconsistent when it comes to types of the buttons, it's just a different pattern. I agree though, that clear all seemed like it's disabled at all times, so @kgabryje added different states for disabled, active and hover, but the idea is to still keep this hierarchy we have and not overuse the colours.
2. I think the hidden scroll bar shouldn't be a problem as it's a natural behaviour/flow. Design wise, scroll bars are usually hidden also in the components that have an internal scroll, not only websites or apps. The gradient that we added should be able to help with it a little bit. It works the same in different parts of app though and in the filter boxes, tables etc.

[kgabryje]

@geido @michael-s-molina I managed to fix the issue with jumping bottom buttons and filter bar container. I used position: fixed on the buttons container, which means now they are detached from filter bar. Filter bar on the other hand is still jumpy... but user doesn't see it because the bottom part is covered by buttons 😄

Screen.Recording.2022-02-21.at.14.26.49.mov

Of course, we can keep looking for a solution that actually prevents the filter bar from jumping, but I think it's a much less pressing issue now.

[geido]

/testenv up

[github-actions]

@geido Ephemeral environment spinning up at http://35.88.112.78:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

[jinghua-qa]

@kgabryje @kasiazjc Thank you for the explanation and I think the new fix for the apply/clear button look better.

I found another cosmetic issue of scrolling up the select box, i think sometimes if user scroll up the bar when select box is open, the select box will be scroll up too, also the content of dashboard is scrolled up

scrolling.issue.mov

[kgabryje]

@kgabryje @kasiazjc Thank you for the explanation and I think the new fix for the apply/clear button look better.

I found another cosmetic issue of scrolling up the select box, i think sometimes if user scroll up the bar when select box is open, the select box will be scroll up too, also the content of dashboard is scrolled up

scrolling.issue.mov

That's super weird 😕 I can't reproduce this issue locally, but I'm fairly sure that it's not related to my changes. @geido @michael-s-molina Are you guys able to reproduce?

[geido]

@kgabryje @kasiazjc Thank you for the explanation and I think the new fix for the apply/clear button look better.
I found another cosmetic issue of scrolling up the select box, i think sometimes if user scroll up the bar when select box is open, the select box will be scroll up too, also the content of dashboard is scrolled up
scrolling.issue.mov

That's super weird 😕 I can't reproduce this issue locally, but I'm fairly sure that it's not related to my changes. @geido @michael-s-molina Are you guys able to reproduce?

@kgabryje @jinghua-qa There is a PR where that issue is being worked on #18102. @pkdotson do you have any update on that issue?

[geido]

/testenv up

[github-actions]

@geido Ephemeral environment spinning up at http://35.87.96.85:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

[geido]

LGTM!

[jinghua-qa]

@kgabryje @kasiazjc Thank you for the explanation and I think the new fix for the apply/clear button look better.
I found another cosmetic issue of scrolling up the select box, i think sometimes if user scroll up the bar when select box is open, the select box will be scroll up too, also the content of dashboard is scrolled up
scrolling.issue.mov

That's super weird 😕 I can't reproduce this issue locally, but I'm fairly sure that it's not related to my changes. @geido @michael-s-molina Are you guys able to reproduce?

@kgabryje @jinghua-qa There is a PR where that issue is being worked on #18102. @pkdotson do you have any update on that issue?

I think you are right, i am also able to reproduce it on master. So it not related to this PR

[jinghua-qa]

LGTM

[github-actions]

Ephemeral environment shutdown and build artifacts deleted.

[shenrie]

@kgabryje Is it at all possible to use CSS to reposition the apply/clear buttons at the top instead of at the bottom, or at least above the set of filters? I have tried changing the flex ordering of the items in the div with the open class, but it doesn't seem to work as I would expect.

The problem that this change has introduced for us, is that when embedding a SS report in an iframe, the height of the iframe affects the location of those buttons so when the dashboard is large in length and we have increased the height of the iframe to accommodate, the user has to unnecessarily scroll down to get to those apply/clear buttons - which is not only annoying but also not intuitive. Ideally, the button container would be anchored to the last filter field and not the bottom of the filter bar.

[shenrie]

@kgabryje Nevermind, I figured out a way to do it with some basic CSS and will leave it here for others in case they also don't like the Apply and Clear buttons at the bottom of the filter box.

Just add this to the dashboard's CSS and modify the top position as needed depending on the number of filtering options are available:

.open div:nth-child(3) {
  
  position: absolute;
  top: 250px;
}

[sfirke]

@shenrie thanks for posting your workaround, I was in the same situation. Once I had 3 filters though it didn't work because it moved the buttons box and the 3rd filter. So I added > to make it non-recursive (only finding direct 3rd children) and it worked:

.open > div:nth-child(3) {
  
  position: absolute;
  top: 300px;
}

[jbat]

@shenrie @sfirke
Have either of you figured out a more flexible design here?
Ideally a way to allow for the addition of Cross Filter to this work around.

If the buttons are fixed, and then multiple Cross Filters are added it means the standard filters slide down below
the fixed position of the buttons resulting in a very non-optimal look.