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-filters): Re-arrange controls in FilterBar #18784

Merged
merged 14 commits into from
Feb 22, 2022

Conversation

kgabryje
Copy link
Member

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

image

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
Copy link

codecov bot commented Feb 17, 2022

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Member

michael-s-molina commented Feb 17, 2022

@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.

Screen Shot 2022-02-17 at 1 44 49 PM

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

@jinghua-qa jinghua-qa added the need:qa-review Requires QA review label Feb 17, 2022
@kgabryje
Copy link
Member Author

@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.

Screen Shot 2022-02-17 at 1 44 49 PM

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
Copy link
Member

michael-s-molina commented Feb 17, 2022

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
Copy link
Member

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
Copy link
Contributor

kasiazjc commented Feb 17, 2022

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
Copy link
Member

@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
Copy link
Contributor

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

@jinghua-qa
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

@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.

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

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

@kgabryje
Copy link
Member Author

/testenv up

@github-actions
Copy link
Contributor

@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
Copy link
Member

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

Screen Shot 2022-02-18 at 12 38 20 PM

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
Copy link
Member Author

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.
image
image

@kasiazjc
Copy link
Contributor

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

Screen Shot 2022-02-18 at 12 38 20 PM

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
Copy link
Member Author

@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
Copy link
Member

geido commented Feb 21, 2022

/testenv up

@github-actions
Copy link
Contributor

@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
Copy link
Member

@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
Copy link
Member Author

@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
Copy link
Member

geido commented Feb 22, 2022

@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
Copy link
Member

geido commented Feb 22, 2022

/testenv up

@github-actions
Copy link
Contributor

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

Copy link
Member

@geido geido left a comment

Choose a reason for hiding this comment

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

LGTM!

@jinghua-qa
Copy link
Member

@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

Copy link
Member

@jinghua-qa jinghua-qa left a comment

Choose a reason for hiding this comment

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

LGTM

@kgabryje kgabryje merged commit 9d5c050 into apache:master Feb 22, 2022
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@shenrie
Copy link

shenrie commented Jun 11, 2022

@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
Copy link

shenrie commented Jun 21, 2022

@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;
}

image

@sfirke
Copy link
Contributor

sfirke commented Aug 16, 2022

@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;
}

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 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 lts-v1 need:qa-review Requires QA review size/L 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants