fix: Aligns top level tabs when native filters are enabled by michael-s-molina · Pull Request #14987 · apache/superset

michael-s-molina · 2021-06-04T14:57:08Z

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

See the original issue for before screenshots.

20210604100632295.mp4

TESTING INSTRUCTIONS

1 - Enable the native filters
2 - Open a dashboard with top-level tabs
3 - Expand the native filters
4 - The top-level tabs should be aligned with the dashboard content

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 · 2021-06-04T15:13:47Z

Codecov Report

Merging #14987 (99879fd) into master (6955ed2) will increase coverage by 0.06%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master   #14987      +/-   ##
==========================================
+ Coverage   77.61%   77.68%   +0.06%     
==========================================
  Files         965      965              
  Lines       49504    49542      +38     
  Branches     6259     6271      +12     
==========================================
+ Hits        38425    38489      +64     
+ Misses      10879    10851      -28     
- Partials      200      202       +2

Flag	Coverage Δ
javascript	`72.64% <75.00%> (+0.15%)`	⬆️

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

Impacted Files	Coverage Δ
...nd/src/dashboard/containers/DashboardComponent.jsx	`92.30% <ø> (ø)`
...end/src/dashboard/components/StickyVerticalBar.tsx	`46.66% <33.33%> (-3.34%)`	⬇️
...d/components/DashboardBuilder/DashboardBuilder.tsx	`86.59% <75.00%> (-0.50%)`	⬇️
...d/src/dashboard/components/gridComponents/Tabs.jsx	`87.50% <100.00%> (+0.12%)`	⬆️
...board/components/nativeFilters/FilterBar/index.tsx	`100.00% <100.00%> (ø)`
...et-frontend/src/dashboard/actions/nativeFilters.ts	`82.69% <0.00%> (+1.84%)`	⬆️
...end/src/filters/components/Range/transformProps.ts	`100.00% <0.00%> (+87.50%)`	⬆️
...src/filters/components/Range/RangeFilterPlugin.tsx	`88.67% <0.00%> (+88.67%)`	⬆️

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 6955ed2...99879fd. Read the comment docs.

kgabryje · 2021-06-07T12:43:49Z

What do you think about making it look like this?

I'm not sure if I like that empty space on the left side of the bar with top level tabs

kgabryje · 2021-06-07T12:45:55Z

+  const tabsPaddingLeft =
+    nativeFiltersEnabled && !editMode && dashboardFiltersOpen
+      ? NATIVE_FILTER_BAR_WIDTH + 20
+      : 8;


Can we use theme variables here? {theme.gridUnit * 2}px

kgabryje · 2021-06-07T12:47:27Z


 const TABS_HEIGHT = 47;
 const HEADER_HEIGHT = 67;
+const NATIVE_FILTER_BAR_WIDTH = 255;


Why 255 instead of 250?

To fix the inner margin-right of the filter values. They have a fixed width.

michael-s-molina · 2021-06-07T12:47:51Z

What do you think about making it look like this?

I'm not sure if I like that empty space on the left side of the bar with top level tabs

I also think this is the best approach but it seems that we have a requirement from a client asking to nest the filters inside a top-level tab. @suddjian @junlincc can you check if we can change this requirement?

kgabryje · 2021-06-07T12:50:42Z

          distanceFromTop: number;
        }) => (
          <Contents
+            id="sticky-vertical-bar"


Do we need that id here?

kgabryje · 2021-06-07T12:58:19Z

I also think this is the best approach but it seems that we have a requirement from a client asking to nest the filters inside a top-level tab. @suddjian @junlincc can you check if we can change this requirement?

I feel like that's a case that we discussed some time ago - that we can't treat native filters like filter box with slightly different UI. I think that native filters are "global" to the dashboard by design and the panel doesn't belong to a single top level tab. Scoping of some filters to a tab have been addressed by highlighting tabs and hiding filters that are out of scope, so we don't need to nest the native filters panel to indicate that it belongs to some tab. Though let's wait for an opinion from product point of view 🙂 cc @junlincc

villebro · 2021-06-07T13:09:22Z

I also think this is the best approach but it seems that we have a requirement from a client asking to nest the filters inside a top-level tab. @suddjian @junlincc can you check if we can change this requirement?

I feel like that's a case that we discussed some time ago - that we can't treat native filters like filter box with slightly different UI. I think that native filters are "global" to the dashboard by design and the panel doesn't belong to a single top level tab. Scoping of some filters to a tab have been addressed by highlighting tabs and hiding filters that are out of scope, so we don't need to nest the native filters panel to indicate that it belongs to some tab. Though let's wait for an opinion from product point of view 🙂 cc @junlincc

The possibility to place native filters on the dashboard layout (similar to Filter Box) is outside the scope of Native Filters - those that want to do that can enable the DASHBOARD_FILTERS_EXPERIMENTAL feature flag. While placing a filter component close to charts within scope can feel intuitive, I personally feel the new in/out of scope logic introduced by #14933 provides a better UX and makes it easier to quickly check which filters are available and in scope.

suddjian · 2021-06-07T17:36:01Z

@michael-s-molina I agree. The design with the filters outside of tabs better communicates to the user how the filters interact with the rest of the application, so we should do that.

junlincc · 2021-06-07T17:52:52Z

@michael-s-molina please proceed this new design, this is much better. 🙏 thank you all for offering suggestions. I think we can merge this PR first though!

junlincc · 2021-06-07T18:17:34Z

@michael-s-molina Hi Michael, dashboard top right tab and dashboard are not aligned perfectly especially on resize. could we address in this or the new design pr?

Screen.Recording.2021-06-07.at.11.16.11.AM.mov

michael-s-molina · 2021-06-07T18:34:42Z

I'm closing this PR and opening a new one with the proposed solution.

Fix: Aligns top level tabs when native filters are enabled

99879fd

superset-github-bot Bot added the Turing label Jun 4, 2021

pull-request-size Bot added the size/M label Jun 4, 2021

michael-s-molina changed the title ~~Fix: Aligns top level tabs when native filters are enabled~~ fix: Aligns top level tabs when native filters are enabled Jun 4, 2021

junlincc added the dashboard:native-filters Related to the native filters of the Dashboard label Jun 4, 2021

junlincc requested review from kgabryje, simcha90 and suddjian June 4, 2021 20:15

kgabryje reviewed Jun 7, 2021

View reviewed changes

junlincc added the hold! On hold label Jun 7, 2021

michael-s-molina closed this Jun 7, 2021

Conversation

michael-s-molina commented Jun 4, 2021

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Uh oh!

codecov Bot commented Jun 4, 2021

Codecov Report

Uh oh!

kgabryje commented Jun 7, 2021

Uh oh!

kgabryje Jun 7, 2021

Choose a reason for hiding this comment

Uh oh!

kgabryje Jun 7, 2021

Choose a reason for hiding this comment

Uh oh!

michael-s-molina Jun 7, 2021

Choose a reason for hiding this comment

Uh oh!

michael-s-molina commented Jun 7, 2021

Uh oh!

kgabryje Jun 7, 2021

Choose a reason for hiding this comment

Uh oh!

kgabryje commented Jun 7, 2021

Uh oh!

villebro commented Jun 7, 2021

Uh oh!

suddjian commented Jun 7, 2021

Uh oh!

junlincc commented Jun 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

junlincc commented Jun 7, 2021

Uh oh!

michael-s-molina commented Jun 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

junlincc commented Jun 7, 2021 •

edited

Loading