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: Move Apply and ClearAll Button below filters and change clearAll Button to link #17505

Conversation

Nithin-George-Philips
Copy link
Contributor

@Nithin-George-Philips Nithin-George-Philips commented Nov 22, 2021

SUMMARY

  • Moved Clear All and Apply Buttons below the filters

New UI

Screenshot 2021-11-23 at 9 01 14 PM

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

@Nithin-George-Philips Nithin-George-Philips changed the title feat:move clearAll and Apply Button below filters and change clearAll Button to link feat: Move clearAll and Apply Button below filters and change clearAll Button to link Nov 23, 2021
@Nithin-George-Philips Nithin-George-Philips changed the title feat: Move clearAll and Apply Button below filters and change clearAll Button to link #feat: Move clearAll and Apply Button below filters and change clearAll Button to link Nov 23, 2021
@Nithin-George-Philips Nithin-George-Philips changed the title #feat: Move clearAll and Apply Button below filters and change clearAll Button to link feat: Move clearAll and Apply Button below filters and change clearAll Button to link Nov 23, 2021
@codecov
Copy link

codecov bot commented Nov 23, 2021

Codecov Report

Merging #17505 (5964373) into master (9576478) will increase coverage by 0.00%.
The diff coverage is 100.00%.

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

@@           Coverage Diff           @@
##           master   #17505   +/-   ##
=======================================
  Coverage   77.00%   77.00%           
=======================================
  Files        1049     1050    +1     
  Lines       56588    56594    +6     
  Branches     7825     7826    +1     
=======================================
+ Hits        43574    43580    +6     
  Misses      12760    12760           
  Partials      254      254           
Flag Coverage Δ
javascript 71.09% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...omponents/nativeFilters/FilterBar/Footer/index.tsx 100.00% <100.00%> (ø)
...omponents/nativeFilters/FilterBar/Header/index.tsx 100.00% <100.00%> (ø)
...board/components/nativeFilters/FilterBar/index.tsx 89.31% <100.00%> (+0.08%) ⬆️

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 9576478...557d682. Read the comment docs.

@Nithin-George-Philips Nithin-George-Philips changed the title feat: Move clearAll and Apply Button below filters and change clearAll Button to link feat: Move Apply and ClearAll Button below filters and change clearAll Button to link Nov 23, 2021
@junlincc
Copy link
Member

Hi @Nithin-George-Philips , first of all, thank you so much for your contribution!
As an engineering driven Open Source project, we love to see contribution from individuals and organization.
However, certain areas of change in the product are more controversial or sensitive than the others. When it comes to UI/UX changes, larger changes that involved altering user flow and behavior generally require posting a SIP(Superset Improvement Proposal), and getting it voted it before starting implementation. For smaller changes like this PR, we requires PR authors to provide clear rationale of change, detailed context or associate open issue. We are also working on the design guideline that contributor can refer to before making a change.

In this case, the reason the “Apply” and “Clear all” buttons are on the top is because the list of filters can become very long, in which case the buttons might be outside the currently visible viewport. In addition, keeping them on top makes it possible to keep them sticky when scrolling the filter list.

If you found the current solution is not sufficient for your use case, we suggest you to either kickoff a discussion in the Issue page, or update the PR description with change rational and we can continue our discussion in the thread.

For now, unfortunately, we will not be able to merge your PR.
If you are looking for area to contribute, we are happy to provide a list of UI/UX started issues as well!

@junlincc junlincc added hold:in-discussion need:consensus Requires consensus need:design-review Requires input/approval from a Designer labels Nov 23, 2021
@amitmiran137
Copy link
Member

amitmiran137 commented Nov 24, 2021

@junlincc I totally agree with the need of keeping the buttons sticky in case of many filters
But in cases when you have only a few filters it make sense to stick it to the bottom as part of a regular user flow ( working top to bottom )

Would you consider that as a configurable feature (either bottom/top ) ?
There would be a native filters configuration and by default it would be on top
In that approach we can make superset flexible to different types of products
In cases of client facing applications there would be a very summerized dashboard with just a few filters but currently superset is more BI/Operational oriented and that make us struggle of making it intuitive to certain clients

cc:@jayakrishnankarolilnlsn

@geido
Copy link
Member

geido commented Nov 24, 2021

I think the problem of moving this to the bottom in a relative position is very clear and I don't think a separate config would be the best way forward. However, there might be another solution, where we make it sticky but at the bottom of the filter sidebar. What do you guys think?

@jayakrishnankk
Copy link
Contributor

I'm thinking in this direction.

  • By default the buttons will be at the top.
  • If configured (need to add a configuration option), it can be shown at the bottom.
  • If shown at the bottom, the "Cancel" and "Apply" buttons will be shown at the end of the available filters until the filters fill the page.
  • If shown at the bottom, and filters fill the page, make the buttons sticky at the bottom so that it is always visible, and scrollbar available to see all filters.

Side note: if a dashboard falls into the last criteria, it is better for the dashboard builder to use the default position for the buttons which is at the top.

Advantages:

  • No breaking change, or UX change in default behavior
  • Configurable option for those who wish to have these buttons at the bottom, because that is a better mental model for most filters. You change the filters and then apply at the end.

@jayakrishnankk
Copy link
Contributor

I had a discussion with @Nithin-George-Philips . We decided on the following steps, that will introduce least changes.

  • Break up the FilterBar > Header element into a Header and a Footer.
  • Put the Footer right below the Header and use css overrides to move the footer to the bottom (advantage: essentially no change in the end user behavior)

Plan is to use dashboard specific css by using the flexbox properties to put the footer to the bottom. Going to test this theory. If it works, then it should be pretty straight forward.

@Nithin-George-Philips Nithin-George-Philips deleted the superset-filtering-UX-changes branch November 27, 2021 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need:consensus Requires consensus need:design-review Requires input/approval from a Designer size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants