[Feature] Dashboard filter indicators by graceguo-supercat · Pull Request #7908 · apache/superset

graceguo-supercat · 2019-07-21T23:36:00Z

SUMMARY

Show indicator on each chart which filter can apply and what filter values are applied
Allow user navigate from chart to any of its applicable filter.

Implementation

Refactor filters in redux state.

Before this PR, filters state is a sub-state in dashboardstate:

dashboard redux state:

{
    datasources,
    sliceEntities,
    charts,
    dashboardInfo,
    dashboardState: {
      ...
      filters: {
        124: {
           __time_range: 'Last quarter',
        },
        131: {
           gender: ['boy', 'girl'],
        }
      }
      ...
    },
    dashboardLayout,
    messageToasts: [],
    impressionId: shortid.generate(),
}

In order to support UI features and in-dashboard navigation functionality, I move dashboardFilter up to be root-level state in redux store, and it will contain more props about filter value and its layout information:

dashboard redux state:

{
    datasources,
    sliceEntities,
    charts,
    dashboardInfo,
    dashboardState,
    dashboardFilters: {
      124: {
        chartId: 124,
        componentId: "CHART-CHTxAXRuJW",
        directPathToFilter: [
          'ROOT_ID',
          'TABS-VPEX_c476g',
          'TAB-1bnpXJkoS8',
          'ROW-qc1WPC4bOn',
          'CHART-CHTxAXRuJW'
        ],
        scope: "ROOT_ID",
        isDateFilter: true,
        isInstantFilter: true,
        columns: {
          __time_range(pin): "Last quarter",
        }
      },
      131: {
        ...
      }
    },
    dashboardLayout,
    messageToasts: [],
    impressionId: shortid.generate(),
}

Because filters prop is used by a lot of components in dashboard, I added a converter utility function buildActiveFilters, which converts raw dashboardFilters state into old filters format so that other components can still use filters as before, and won't be affected by this big state change.

buildActiveFilters function will be triggered whenever dashboardFilters state is changed, and the converted results will be cached. All other components in dashboard will call getActiveFilters to get the cached filters state, and they don't need to run the converter function again and again.

Color assignment

In this PR we introduced a new color palette for dashboard filters.

Each filter field is assigned a unique colorKey: chartId_filter_column_name. This colorKey is used by bother filter badge and filter indicator. Dashboard will pick a color from filter color palette based on colorKey and build a color map. During render time, each filter and indicators in the chart will lookup color code based on its colorKey. Similar to buildActiveFilters function, dashboardFilter state update will trigger re-build color map.

In-dashboard navigation

Suppose A is root, B,C,D are top level tabs, F, G are charts under tabs, H is nested tab, M is chart under nested tab. For example, when user lands on M, how to navigate user to a filter component in another root level tab B?

Dashboard stores layout data in flattened tree structure: each parent node contains ids of its children, and every UI component (chart, header, row, column, tabs, etc) keeps all the parent component ids from root to itself (see #6945):

  'CHART-PuHBPak5nl': {
    children: [],
    id: 'CHART-PuHBPak5nl',
    meta: {
      chartId: 146,
      height: 50,
      sliceName: 'Chart B2',
      width: 4
    },
    parents: [
      'ROOT_ID',
      'TABS-nGgMw9Fmmj',
      'TAB-YWfqbvBaqp',
      'TABS-cGWCi1V-mr',
      'TAB-CZxYwmgZZ',
      'ROW-iC9LI5XSxU'
    ],
    type: 'CHART'
  }

From this parents list it's very easy to find out to select which root tab or nested tab to a specific filter. dashboardState has a directPathToChild prop, which is the current state of dashboard, holding a path to one of the chart (or any UI component). When user clicks on a filter indicator, dashboard will update its directPathToChild to be target filter's path. Then dashboard will render the correct list of parent tab/tabs, and display filter chart.

Filter indicators UI components
- Container component: /dashboard/container/FilterIndicators.jsx
- /dashboard/components/FilterIndicatorsContainer.jsx: indicators organizer for each chart. Usually one chart will be applicable for one or more filters, but user can set dashboard metadata that a chart might be immune to all filters in the dashboard, or some filter fields. We only display indicators for applicable filters. The logic to show/group/hide applicable filters for each chart is here.
- /dashboard/components/FilterIndicator.jsx and /dashboard/components/FilterIndicatorGroup.jsx: little colored indicator component for each filter, if chart having > 5 filters, will group them into a group.

/dashboard/components/FilterTooltipWrapper.jsx: the basic usage of react-bootstrap Tooltip components are not enough for our need. We allow user to click on tooltip (especially needed for indicator group) to navigate to actual filter chart. So I added customized wrapper component to trigger/hide Overlay with some delay.
/dashboard/components/FilterIndicatorTooltip.jsx: tooltip component for filter. it shows column name and selected values.

If the filter is applicable but not applied, we will show **Not filtered**. If the filter is not applicable to the chart, we will not show indicator.

SCREENSHOTS OR ANIMATED GIF

Page landing:

each filter field is assigned a badge with unique color.

Interact with filter indicator

Hover on chart, you will see filter indicators popping out. The color bar in the filter indicator matched with filter's badge color.
Hover on filter indicator, it will show filter's name and selected value(s), as filter tooltip.
We show at most 5 filter indicators for each chart. For charts that having more filters, its indicators will be grouped in the last spot.
For immune chart, it doesn't have any filter indicator.
For chart with immune fields, we only show indicators for applicable fields.

Navigate from chart to filter

When user clicks on one of filter indicator, or click on pencil icon in the filter tooltip, page will navigate to the chart, even cross tabs.
The selected filter chart will be highlighted with green border.
If the chart has more than 1 field, the user selected field will be highlighted.

TEST PLAN

CI and manual test.

ADDITIONAL INFORMATION

REVIEWERS

@williaster @michellethomas @etr2460 @xtinec @mistercrunch @kenchendesign
@betodealmeida @khtruong

villebro · 2019-07-22T06:00:09Z

Needs rebasing (build error fixed in master)

codecov-io · 2019-07-22T15:23:11Z

Codecov Report

Merging #7908 into master will increase coverage by 0.11%.
The diff coverage is 71.09%.

@@            Coverage Diff             @@
##           master    #7908      +/-   ##
==========================================
+ Coverage   65.57%   65.69%   +0.11%     
==========================================
  Files         469      486      +17     
  Lines       22496    22804     +308     
  Branches     2446     2515      +69     
==========================================
+ Hits        14752    14980     +228     
- Misses       7624     7696      +72     
- Partials      120      128       +8

Impacted Files	Coverage Δ
...ets/src/visualizations/FilterBox/transformProps.js	`0% <ø> (ø)`	⬆️
.../assets/src/dashboard/containers/DashboardGrid.jsx	`100% <ø> (ø)`	⬆️
superset/assets/src/chart/chartReducer.js	`20.33% <ø> (ø)`	⬆️
superset/assets/src/dashboard/reducers/index.js	`100% <ø> (ø)`	⬆️
...rset/assets/src/dashboard/containers/Dashboard.jsx	`0% <ø> (ø)`	⬆️
...ssets/src/dashboard/containers/DashboardHeader.jsx	`100% <ø> (ø)`	⬆️
...sets/src/dashboard/containers/DashboardBuilder.jsx	`0% <ø> (ø)`	⬆️
...ts/src/dashboard/containers/DashboardComponent.jsx	`100% <ø> (ø)`	⬆️
...t/assets/src/dashboard/reducers/getInitialState.js	`0% <0%> (ø)`	⬆️
...et/assets/src/dashboard/reducers/dashboardState.js	`73.46% <0%> (-9.87%)`	⬇️
... and 55 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 4e7ea3f...b02b9a9. Read the comment docs.

etr2460

a bunch of comments, will definitely give another pass too

etr2460 · 2019-07-22T16:34:17Z

superset/assets/src/dashboard/components/Dashboard.jsx

camelCase please

etr2460 · 2019-07-22T16:34:28Z

superset/assets/src/dashboard/components/Dashboard.jsx

etr2460 · 2019-07-22T16:34:36Z

superset/assets/src/dashboard/components/Dashboard.jsx

etr2460 · 2019-07-22T16:35:52Z

superset/assets/spec/javascripts/dashboard/actions/dashboardState_spec.js

nit: should be 'should dispatch removeFilter i think

etr2460 · 2019-07-22T16:39:02Z

superset/assets/spec/javascripts/dashboard/components/Dashboard_spec.jsx

maybe just a stylistic preference, but i tend to name constant fixtures like this as OVERRIDE_FILTERS

etr2460 · 2019-07-22T18:31:03Z

superset/assets/src/dashboard/util/dashboardFiltersColorMap.js

maybe note this var in the css file (and vice versa) so that people know where to update it if something changes

etr2460 · 2019-07-22T18:32:15Z

superset/assets/src/dashboard/util/getFilterConfigsFromFormdata.js

camelCase throughout please!

etr2460 · 2019-07-22T18:33:45Z

superset/assets/src/dashboard/util/getFilterConfigsFromFormdata.js

i'd prefer to not disable the lint rule here and just make an additional const in this function if needed

etr2460 · 2019-07-22T18:35:08Z

superset/assets/src/dashboard/util/isEmptyValue.js

i'd imagine there's a lodash function or something that already implements this, any reason why we chose to write our own here?

thanks! Use lodash now.

etr2460 · 2019-07-22T18:35:48Z

superset/assets/src/visualizations/FilterBox/FilterBox.jsx

should this be wrapped in t()?

we don't translate values in map. the UI component which uses this map will translate the needed value.

graceguo-supercat · 2019-08-01T17:30:15Z

ping @etr2460 I rebased onto master, added a few unit tests to increase coverage (you have to click the Codecov link to see updated stats), fixed most of comments. Please take a second look. Thanks.

etr2460

a few more comments!

etr2460 · 2019-08-05T22:56:41Z

superset/assets/src/components/AnchorLink.jsx

You don't need this function anymore right?

thanks. i added this instance method before, because I had difficulty to add unit test, which needs to mock window.location.hash. now i found a better solution here.

etr2460 · 2019-08-05T23:07:03Z

superset/assets/src/dashboard/components/FilterIndicatorGroup.jsx

personal preference, but i find it easier to read if we just put this map in the return JSX as well. Co-locating all of it makes it easier to see the whole dom structure

etr2460 · 2019-08-05T23:07:48Z

superset/assets/src/dashboard/components/FilterIndicatorTooltip.jsx

still not sure if this is needed?

etr2460 · 2019-08-05T23:22:53Z

superset/assets/src/dashboard/reducers/dashboardFilters.js

prefer: const { [action.chartId]: deletedItem, ...updatedFilters };

https://github.com/airbnb/javascript/blob/master/README.md#objects--rest-spread

thanks! this problem bothered me for a long time. now i knew solution.

etr2460 · 2019-08-05T23:25:53Z

superset/assets/src/dashboard/stylesheets/filter-indicator.less

is a z index absolutely needed here? Have we ensured that 10 is appropriate compared to all other possible z indexes?

be honest, i don't know how to make ensured 10 is appropriate. Just by manual test.

got it, we should probably make an issue to manage the z-indexes better at some point. this is fine for now though

etr2460 · 2019-08-05T23:26:24Z

superset/assets/src/dashboard/stylesheets/filter-indicator.less

not a big fan of transition all, can we explicitly call out what styles should be transitioned over?

fixed. only width attribute.

etr2460

just a couple final nits, but otherwise lgtm! I'm super excited to see this go out!

etr2460 · 2019-08-07T23:12:32Z

superset/assets/src/components/AnchorLink.jsx

this doesn't need an argument right? or you can remove the default arg on the function

etr2460 · 2019-08-07T23:15:56Z

superset/assets/src/dashboard/components/FilterIndicatorGroup.jsx

We're using react 16.8 now so this can be changed to <> and the closing tag can be </>

I search the whole code base and found about 9 places used <React.Fragment> but 0 place used <>. Maybe consistency is more important, if this is only a syntax sugar?

etr2460 · 2019-08-07T23:17:08Z

superset/assets/src/dashboard/reducers/dashboardFilters.js

etr2460 · 2019-08-07T23:51:42Z

superset/assets/src/dashboard/stylesheets/filter-indicator.less

got it, we should probably make an issue to manage the z-indexes better at some point. this is fine for now though

mistercrunch · 2019-08-08T04:17:35Z

I can't wait to see this through. Hats off to everyone involved!

tvorogme · 2019-08-13T10:35:46Z

Any updates? Very cool PR....

etr2460 · 2019-08-13T15:49:27Z

This will probably get merged today!

tvorogme · 2019-08-15T07:39:19Z

YAY!!!!!

pull-request-size bot added the size/XXL label Jul 21, 2019

graceguo-supercat force-pushed the gg-FilterIndicator branch from 89184f3 to 2e92ec9 Compare July 22, 2019 06:40

etr2460 requested changes Jul 22, 2019

View reviewed changes

graceguo-supercat mentioned this pull request Jul 23, 2019

[dashboard] fix chart showing loading icon when slice's immune fields is changed #7895

Merged

6 tasks

graceguo-supercat changed the title ~~[Feature] Dashboard filter indicators~~ [WiP][Feature] Dashboard filter indicators Jul 31, 2019

graceguo-supercat force-pushed the gg-FilterIndicator branch 2 times, most recently from ea70f42 to 568ea9c Compare August 1, 2019 06:01

graceguo-supercat changed the title ~~[WiP][Feature] Dashboard filter indicators~~ [Feature] Dashboard filter indicators Aug 1, 2019

graceguo-supercat force-pushed the gg-FilterIndicator branch from 568ea9c to 4f394b1 Compare August 3, 2019 00:04

etr2460 requested changes Aug 5, 2019

View reviewed changes

graceguo-supercat force-pushed the gg-FilterIndicator branch 2 times, most recently from b318ea2 to f0be2fc Compare August 6, 2019 19:16

etr2460 approved these changes Aug 7, 2019

View reviewed changes

graceguo-supercat force-pushed the gg-FilterIndicator branch from f0be2fc to 9ab3e38 Compare August 8, 2019 17:53

Grace Guo added 2 commits August 18, 2019 23:50

dashboard filter indicators

649fcc7

add/fix unit tests

b02b9a9

graceguo-supercat force-pushed the gg-FilterIndicator branch from 9ab3e38 to b02b9a9 Compare August 19, 2019 06:57

graceguo-supercat merged commit 40776bd into apache:master Aug 22, 2019

graceguo-supercat mentioned this pull request Sep 4, 2019

[feature][dashboard] Show/hide filter indicator on the applicable charts when filter options are open/close #8166

Merged

12 tasks

graceguo-supercat deleted the gg-FilterIndicator branch September 21, 2019 08:09

graceguo-supercat mentioned this pull request Oct 4, 2019

[dashboard] show fewer filter indicators #8345

Merged

12 tasks

graceguo-supercat mentioned this pull request Apr 22, 2020

fix: re-enable table filter #9593

Closed

3 tasks

mistercrunch added the 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels label Feb 28, 2024

mistercrunch added the 🚢 0.35.0 First shipped in 0.35.0 label Feb 28, 2024

Conversation

graceguo-supercat commented Jul 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CATEGORY

SUMMARY

Implementation

SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

REVIEWERS

Uh oh!

villebro commented Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

etr2460 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

graceguo-supercat Jul 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

graceguo-supercat commented Aug 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

etr2460 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

graceguo-supercat commented Jul 21, 2019 •

edited

Loading

villebro commented Jul 22, 2019 •

edited

Loading

codecov-io commented Jul 22, 2019 •

edited

Loading

graceguo-supercat Jul 30, 2019 •

edited

Loading

graceguo-supercat commented Aug 1, 2019 •

edited

Loading

graceguo-supercat Aug 6, 2019 •

edited

Loading