feat(native-filters): Highlight charts affected by focused native filter #14693

kgabryje · 2021-05-18T16:10:16Z

SUMMARY

When user focuses on a native filter (hovers over it or opens select menu), highlight all charts in scope of that native filter (by adding a box shadow around it) and hide the charts outside of scope (by setting opacity to 0.3).

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2021-05-18.at.17.54.47.mov

TEST PLAN

Set DASHBOARD_NATIVE_FILTERS feature flag to true.
Add some native filters to the dashboard - be creative with setting their scopes 🙂
Verify that when you hover over Range or Time filter buttons, charts inside the scopes of those filters get highlighted and all other charts are less visible.
Verify that when you open a select menu of Select, Time Column, Time Grain and Group By filters, charts inside the scopes of those filters get highlighted and all other charts are less visible.

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

CC: @junlincc @villebro @suddjian

graceguo-supercat · 2021-05-18T18:12:55Z

Could you create a sample dashboard with some Tabs, and set filter scope that is only applicable to those charts inside tab?

codecov · 2021-05-18T22:25:22Z

Codecov Report

Merging #14693 (74c4ec5) into master (32f5f36) will decrease coverage by 0.04%.
The diff coverage is 43.10%.

@@            Coverage Diff             @@
##           master   #14693      +/-   ##
==========================================
- Coverage   77.51%   77.46%   -0.05%     
==========================================
  Files         958      959       +1     
  Lines       48560    48733     +173     
  Branches     5703     6115     +412     
==========================================
+ Hits        37642    37752     +110     
- Misses      10718    10778      +60     
- Partials      200      203       +3

Flag	Coverage Δ
javascript	`72.45% <43.10%> (-0.07%)`	⬇️

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

Impacted Files	Coverage Δ
...components/DashboardBuilder/DashboardContainer.tsx	`100.00% <ø> (ø)`
...ashboard/components/gridComponents/ChartHolder.jsx	`77.77% <ø> (+0.94%)`	⬆️
...nents/nativeFilters/FilterBar/FilterSets/index.tsx	`77.00% <ø> (ø)`
superset-frontend/src/dashboard/reducers/types.ts	`0.00% <ø> (ø)`
...filters/components/GroupBy/GroupByFilterPlugin.tsx	`0.00% <0.00%> (ø)`
...d/src/filters/components/GroupBy/transformProps.ts	`22.22% <0.00%> (-11.12%)`	⬇️
...t-frontend/src/filters/components/GroupBy/types.ts	`100.00% <ø> (ø)`
...src/filters/components/Range/RangeFilterPlugin.tsx	`0.00% <0.00%> (ø)`
...end/src/filters/components/Range/transformProps.ts	`12.50% <0.00%> (-7.50%)`	⬇️
...et-frontend/src/filters/components/Select/types.ts	`100.00% <ø> (ø)`
... and 48 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 32f5f36...74c4ec5. Read the comment docs.

junlincc · 2021-05-18T23:41:20Z

@kgabryje i believe in the original implementation, when there are multiple tabs, tabs light up if any charts in that tab is mapped to a specific filter.

kgabryje · 2021-05-19T09:51:20Z

@graceguo-supercat @junlincc Thank you for your comments! The charts inside of tabs are correctly highlighted. However, if a chart in filter's scope is in an inactive tab, that tab doesn't get highlighted - I'll try to implement that.
@junlincc Are you sure that tabs were getting highlighted? I don't see the relevant code and there are no tabs highlights in the gif from Grace's comment here #10936 (comment)

kgabryje · 2021-05-19T10:25:50Z

superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx

@@ -72,7 +72,7 @@ const DashboardContainer: FC<DashboardContainerProps> = ({ topLevelTabs }) => {
            activeKey={activeKey}
            renderTabBar={() => <></>}
            fullWidth={false}
-            animated
+            animated={false}


I disabled animation as it was breaking native filters - when user switched tab, margin-left: 100% style was set on tab content, which was covering native filters panel, causing the user to be unable to click native filters... And the animation looked ugly with native filters panel anyway 😛

junlincc · 2021-05-19T14:50:26Z

Are you sure that tabs were getting highlighted?

got it, that was in the design but never implemented....😅 ;let's spend a day or 2 to add this feature in a seperate PR

highlight other tabs that have charts scoped to the filter, when user is in a different tab.
when user switch to the highlighted tab, user see highlighted charts in tab
please open a separate PR to disable animation in tab globally @ktmud
please open a separate PR to grey out filters that are not scoped to a specific tag.

graceguo-supercat · 2021-05-19T16:31:00Z

@junlincc Are you sure that tabs were getting highlighted? I don't see the relevant code and there are no tabs highlights in the gif from Grace's comment here #10936 (comment)

For filter_box, we don't highlight tabs when only charts inside inactive tab will be impacted. We had this behavior is because filter_box could be placed inside tab. So that if a filter_box only has smaller tab scope, it should be placed inside the tab. It is possible that user sets a filter_box is applicable to multiple tabs but hide the filter_box inside a nested tab, but this case is small (why would someone do that?)

For build-in filter components, things are a little different: every filter component become globally visible in the dashboard, not matter it is applicable to single nested tab or multiple top-level tabs. This change might need us to think about the highlight behavior:

When a filter component is applicable to a nested tab and currently user can not see the nested tab, filter component should be inactive (not in the current implementation but i assume this is in the near future), i think we still should highlight the scope even for the inactive filter component, so that user will know why the filter is not active now and what is its real scope. We should highlight the parent tab which is visible for the user when the charts are deep nested under other tabs.
When a filter component is applicable to one more charts/tabs in the current active tab, filter component is active, we should highlight charts and tabs, so that behavior is consistent with above case.

want to hear ideas from @junlincc and @mihir174

villebro

A few non-blocking comments, LGTM!

villebro · 2021-05-20T12:30:39Z

superset-frontend/src/dashboard/actions/nativeFilters.ts

-export interface SetBooststapData {
+export interface SetBootstrapData {


This gave me fond memories of C++ https://www.boost.org/

😆 Ah boost... I remember that I was so happy when C++11 (I think?) was released and they added a lot of stuff from boost to std 😄

villebro · 2021-05-20T14:11:05Z

superset-frontend/src/filters/components/GroupBy/types.ts

+  setFocusedFilter: () => void;
+  unsetFocusedFilter: () => void;


Simplification idea: we could move the setDataMask, setFocusedFilter and unsetFocusedFilter into a shared interface/type PluginFilterHooks in filters/components/types.ts and then just & them into PluginFilterGroupByProps and the others.

Good point, done!

villebro · 2021-05-20T14:11:54Z

superset-frontend/src/filters/components/common.ts

+  height: ${({ height }) => height}px;
+  width: ${({ width }) => width}px;


oh one of these again 👍

junlincc · 2021-05-20T19:58:27Z

@graceguo-supercat

for current iteration - we will grey out filters that are not scoped to tabs from the filter bar, as just a visual suggestion that the specific filter won’t apply in the current scope(instead of disabling it). This should reach feature parity with filter box highlighting behavior

For range filter, blue circle will be greyed out; for time range, color darker grey on hovering

Above change will be made in separate PRs

We can discuss next iteration, including grouping or foldering filters to tabs(but not now). 🙏

@kgabryje please go ahead and merge your PR, I will take a deeper look once multiple native filter PRs are all merged.
cc @michael-s-molina

junlin-qa

approving a batch of native filter PRs, will test all as a whole ASAP.

…ter (apache#14693) * Highlight charts affected by focused native filter * Remove tabs animation on dashboard * Remove a test that checks for "animated={true}" prop on tabs * Move hooks types to a separate interface (cherry picked from commit c831655)

pseudoPixels · 2021-08-04T02:18:28Z

This feature is very intuitive and useful for dashboard users.
However, for some reason this feature is not working for our case (using Superset 1.2). Is there any special feature flag or so that we need to set to enable this? We do not get any error log/info, not sure whats we are missing. Can anyone please hint on it for possible issue? Many thanks all :)

villebro · 2021-08-04T07:30:14Z

This feature is very intuitive and useful for dashboard users.
However, for some reason this feature is not working for our case (using Superset 1.2). Is there any special feature flag or so that we need to set to enable this? We do not get any error log/info, not sure whats we are missing. Can anyone please hint on it for possible issue? Many thanks all :)

I don't believe this PR made it into 1.2 - however, this should be available in 1.3 (forthcoming)

…ter (apache#14693) * Highlight charts affected by focused native filter * Remove tabs animation on dashboard * Remove a test that checks for "animated={true}" prop on tabs * Move hooks types to a separate interface

Highlight charts affected by focused native filter

59bdf14

kgabryje requested review from suddjian and villebro May 18, 2021 16:10

pull-request-size bot added the size/L label May 18, 2021

junlincc requested a review from graceguo-supercat May 18, 2021 16:39

junlincc added release:note dashboard:native-filters Related to the native filters of the Dashboard labels May 18, 2021

junlincc added the demo label May 18, 2021

kgabryje closed this May 19, 2021

kgabryje reopened this May 19, 2021

Remove tabs animation on dashboard

759891d

kgabryje commented May 19, 2021

View reviewed changes

junlincc added the need:review Requires a code review label May 19, 2021

Remove a test that checks for "animated={true}" prop on tabs

a2e0bed

villebro approved these changes May 20, 2021

View reviewed changes

Move hooks types to a separate interface

74c4ec5

junlin-qa approved these changes May 20, 2021

View reviewed changes

kgabryje merged commit c831655 into apache:master May 21, 2021

graceguo-supercat mentioned this pull request May 21, 2021

[native filter] highlight charts do not work #14757

Closed

junlincc added this to In progress in Native dashboard filters via automation May 27, 2021

junlincc moved this from In progress to Done in Native dashboard filters May 27, 2021

junlincc removed the demo label May 28, 2021

junlincc added the dashboard:indicator Related to the filter indicator of a Dashboard label May 28, 2021

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(native-filters): Highlight charts affected by focused native filter #14693

feat(native-filters): Highlight charts affected by focused native filter #14693

kgabryje commented May 18, 2021

graceguo-supercat commented May 18, 2021 •

edited

Loading

codecov bot commented May 18, 2021 •

edited

Loading

junlincc commented May 18, 2021 •

edited

Loading

kgabryje commented May 19, 2021 •

edited

Loading

kgabryje May 19, 2021

junlincc commented May 19, 2021

graceguo-supercat commented May 19, 2021 •

edited

Loading

villebro left a comment

villebro May 20, 2021

kgabryje May 20, 2021

villebro May 20, 2021

kgabryje May 20, 2021

villebro May 20, 2021

junlincc commented May 20, 2021 •

edited

Loading

junlin-qa left a comment

pseudoPixels commented Aug 4, 2021

villebro commented Aug 4, 2021

		export interface SetBooststapData {
		export interface SetBootstrapData {

		setFocusedFilter: () => void;
		unsetFocusedFilter: () => void;

		height: ${({ height }) => height}px;
		width: ${({ width }) => width}px;

feat(native-filters): Highlight charts affected by focused native filter #14693

feat(native-filters): Highlight charts affected by focused native filter #14693

Conversation

kgabryje commented May 18, 2021

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

graceguo-supercat commented May 18, 2021 • edited Loading

codecov bot commented May 18, 2021 • edited Loading

Codecov Report

junlincc commented May 18, 2021 • edited Loading

kgabryje commented May 19, 2021 • edited Loading

kgabryje May 19, 2021

Choose a reason for hiding this comment

junlincc commented May 19, 2021

graceguo-supercat commented May 19, 2021 • edited Loading

villebro left a comment

Choose a reason for hiding this comment

villebro May 20, 2021

Choose a reason for hiding this comment

kgabryje May 20, 2021

Choose a reason for hiding this comment

villebro May 20, 2021

Choose a reason for hiding this comment

kgabryje May 20, 2021

Choose a reason for hiding this comment

villebro May 20, 2021

Choose a reason for hiding this comment

junlincc commented May 20, 2021 • edited Loading

junlin-qa left a comment

Choose a reason for hiding this comment

pseudoPixels commented Aug 4, 2021

villebro commented Aug 4, 2021

graceguo-supercat commented May 18, 2021 •

edited

Loading

codecov bot commented May 18, 2021 •

edited

Loading

junlincc commented May 18, 2021 •

edited

Loading

kgabryje commented May 19, 2021 •

edited

Loading

graceguo-supercat commented May 19, 2021 •

edited

Loading

junlincc commented May 20, 2021 •

edited

Loading