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

graceguo-supercat · 2019-09-04T00:09:29Z

SUMMARY

This is part of dashboard filter indicator design:

when user clicks on indicator, dashboard will navigate to the actual filter_box (done in [Feature] Dashboard filter indicators #7908)
when user open options on a filter_box, the indicators in applicable charts should show up. (thanks for upgrade in feat: bump superset-ui dependencies to v0.12 and plugins to v0.11 #8115 ). See demo below:

This PR also fixed a couple of issue after #7908 is released:

To keep filter name in single line: show ellipsis for long filter name in tooltip. Still show full content for filter options values.

Show color bar when filter has a selected value(s):

when user clicks filter indicator, we used to outline the filter_box. This outline will draw user attention, but after user worked on other area in the dashboard, I feel it is a little annoying with outline still on the old filter_box. So in this PR i make the outline only on about 2 seconds, and will turn off by itself.

TEST PLAN

ADDITIONAL INFORMATION

REVIEWERS

@kristw @etr2460 @kenchendesign

etr2460 · 2019-09-05T17:10:40Z

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

@@ -56,12 +62,14 @@ class FilterIndicator extends React.PureComponent {
    return (
      <FilterTooltipWrapper tooltip={filterTooltip}>
        <div
-          className="filter-indicator"
+          className={`filter-indicator ${
+            isFilterFieldActive ? 'isFilterFieldActive' : ''


this css class naming seems inconsistent, is there anything wrong with just adding on the class active along with filter-indicator? Or at least using dash case to match filter-indicator

now use active.

etr2460 · 2019-09-05T17:12:27Z

superset/assets/src/dashboard/components/gridComponents/ChartHolder.jsx

-  getChartAndLabelComponentIdFromPath() {
-    const { directPathToChild = [] } = this.props;
-    const result = {};
+  UNSAFE_componentWillReceiveProps(nextProps) {


instead of adding a new UNSAFE function, can we use one of the other recommendations here? https://reactjs.org/docs/react-component.html#unsafe_componentwillreceiveprops

now use getDerivedStateFromProps(props, state). Thanks!

superset/assets/src/dashboard/util/propShapes.jsx

etr2460 · 2019-09-05T17:15:14Z

superset/assets/src/explore/components/controls/DateFilterControl.jsx

      this.setTypeCustomStartEnd();
    }
+
+    // if user click outside popover, popover will hide and we will call onCloseDateFilterControl,
+    // but popover overlay trigger button is inside popover DOM h


looks like this comment didn't get finished?

😓 fixed, and added more comments.

kristw · 2019-09-05T18:15:41Z

superset/assets/src/visualizations/FilterBox/transformProps.js

@@ -53,6 +53,8 @@ export default function transformProps(chartProps) {
    filtersFields,
    instantFiltering,
    onChange: onAddFilter,
+    onFilterMenuOpen: hooks.onFilterMenuOpen,


You could destructure these two handlers along with onAddFilter on line 30 above.

kristw · 2019-09-05T18:16:47Z

superset/assets/src/explore/components/controls/DateFilterControl.jsx

@@ -338,7 +365,7 @@ export default class DateFilterControl extends React.Component {
    });
    return (
      <Popover id="filter-popover" placement="top" positionTop={0}>
-        <div style={{ width: '250px' }}>
+        <div style={{ width: '250px' }} ref={(ref) => { this.popoverContainer = ref; }}>


can make style constant outside of class so it won't create new object every time.

i fixed one place. there are quite a few inline css in this file and in the code base. we probably need to find a rule or consistent way to style component.

codecov-io · 2019-09-07T06:07:18Z

Codecov Report

Merging #8166 into master will decrease coverage by 0.01%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master    #8166      +/-   ##
==========================================
- Coverage   66.16%   66.14%   -0.02%     
==========================================
  Files         479      480       +1     
  Lines       22920    23002      +82     
  Branches     2524     2549      +25     
==========================================
+ Hits        15164    15214      +50     
- Misses       7622     7653      +31     
- Partials      134      135       +1

Impacted Files	Coverage Δ
superset/assets/src/dashboard/util/propShapes.jsx	`100% <ø> (ø)`	⬆️
...t/assets/src/dashboard/reducers/getInitialState.js	`0% <ø> (ø)`	⬆️
superset/assets/src/dashboard/containers/Chart.jsx	`91.66% <ø> (ø)`	⬆️
superset/assets/src/chart/Chart.jsx	`10.63% <0%> (-0.48%)`	⬇️
superset/assets/src/chart/ChartRenderer.jsx	`7.14% <0%> (-0.22%)`	⬇️
...ets/src/visualizations/FilterBox/transformProps.js	`0% <0%> (ø)`	⬆️
.../assets/src/visualizations/FilterBox/FilterBox.jsx	`5.93% <0%> (-0.68%)`	⬇️
...ssets/src/dashboard/components/FilterIndicator.jsx	`100% <100%> (ø)`	⬆️
...et/assets/src/dashboard/reducers/dashboardState.js	`76.36% <100%> (+2.89%)`	⬆️
.../src/dashboard/components/FilterIndicatorGroup.jsx	`100% <100%> (ø)`	⬆️
... and 11 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 7546ea3...8894641. Read the comment docs.

etr2460

just a couple more comments

etr2460 · 2019-09-09T20:56:31Z

superset/assets/src/dashboard/components/gridComponents/Chart.jsx

+  }
+
+  handleFilterMenuClose() {
+    this.props.setFocusedFilterField();


not sure if this is super clear, might prefer unsetFocusedFilterField or perhaps setFocusedFilterField(null)?

the purpose of empty parameter here is to clear/unset focused filter field state. Reducer function expects to handle this case.
I thought we prefer no params than explicitly null params? I rarely see any example in our code that explicitly pass a null to function.

updated: added unsetFocusedFilterField action creator.

etr2460 · 2019-09-09T20:57:33Z

superset/assets/src/dashboard/components/gridComponents/ChartHolder.jsx

    return (
      <style>
-        {`.inFocus label[for=${labelName}] + .Select .Select-control {
-                    border: 2px solid #00736a;
+        {`label[for=${columnName}] + .Select .Select-control {


Why is it again that we need to add inline style elements to the react element?

As an alternative, I can pass the selected filter field as prop to filter_box component. But first, it is deeply nested inside Chart component. And since it is a data visualization component, its props needs to be validated from superset-ui chart props. Overall it will be 100 times more troublesome than 2 lines of inline css. :)

by the way this is update existed inline style, not adding new.

seems legit

etr2460 · 2019-09-09T20:58:19Z

superset/assets/src/dashboard/components/gridComponents/ChartHolder.jsx

    };
+    // window.clearTimeout's key is global,
+    // we don't want one chart clear out another's chart's timeout.
+    this.timer = [];


what type is timer? It's not apparent to me that this should be an array

not necessary to keep an instance variable for timer. removed.

etr2460

lgtm, thanks for addressing all the comments!

pull-request-size bot added the size/L label Sep 4, 2019

graceguo-supercat changed the title ~~[feature][dashboard] Show filter indicator on the applicable charts when filter is open~~ [feature][dashboard] Show/hide filter indicator on the applicable charts when filter options are open/close Sep 4, 2019

graceguo-supercat requested review from kristw and etr2460 September 5, 2019 03:11

etr2460 requested changes Sep 5, 2019

View reviewed changes

kristw approved these changes Sep 5, 2019

View reviewed changes

Grace Guo added 3 commits September 6, 2019 22:46

show ellipsis for long filter name in tooltip

c0aa1e2

show filter indicator color bar when filter is applied

14bd83d

show/hide filter indicator when filter is showing/hiding options

feb7049

graceguo-supercat force-pushed the gg-showFilterIndicator branch from 432ba0f to 956cb9f Compare September 7, 2019 05:47

graceguo-supercat requested a review from etr2460 September 7, 2019 05:57

graceguo-supercat force-pushed the gg-showFilterIndicator branch 2 times, most recently from d0e4d38 to 4fc48e5 Compare September 7, 2019 06:55

use component local state to hide/show chart outline

fa1cf96

graceguo-supercat force-pushed the gg-showFilterIndicator branch 2 times, most recently from c1ae6d4 to eff509c Compare September 7, 2019 22:58

etr2460 reviewed Sep 9, 2019

View reviewed changes

graceguo-supercat force-pushed the gg-showFilterIndicator branch from eff509c to 8894641 Compare September 10, 2019 00:03

fix review comments + unit tests

af5e299

graceguo-supercat force-pushed the gg-showFilterIndicator branch from 8894641 to af5e299 Compare September 10, 2019 21:51

etr2460 approved these changes Sep 10, 2019

View reviewed changes

graceguo-supercat merged commit b4a1234 into apache:master Sep 10, 2019

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

graceguo-supercat mentioned this pull request Apr 22, 2020

fix: re-enable table filter #9593

Closed

3 tasks

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

graceguo-supercat commented Sep 4, 2019 •

edited

etr2460 Sep 5, 2019

graceguo-supercat Sep 7, 2019

etr2460 Sep 5, 2019

graceguo-supercat Sep 7, 2019

etr2460 Sep 5, 2019

graceguo-supercat Sep 7, 2019

kristw Sep 5, 2019

graceguo-supercat Sep 7, 2019

kristw Sep 5, 2019

graceguo-supercat Sep 7, 2019

codecov-io commented Sep 7, 2019 •

edited

etr2460 left a comment

etr2460 Sep 9, 2019

graceguo-supercat Sep 10, 2019 •

edited

graceguo-supercat Sep 10, 2019 •

edited

etr2460 Sep 9, 2019

graceguo-supercat Sep 10, 2019 •

edited

etr2460 Sep 10, 2019

etr2460 Sep 9, 2019

graceguo-supercat Sep 10, 2019

etr2460 left a comment

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

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

Conversation

graceguo-supercat commented Sep 4, 2019 • edited

CATEGORY

SUMMARY

TEST PLAN

ADDITIONAL INFORMATION

REVIEWERS

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Sep 7, 2019 • edited

Codecov Report

etr2460 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graceguo-supercat Sep 10, 2019 • edited

Choose a reason for hiding this comment

graceguo-supercat Sep 10, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graceguo-supercat Sep 10, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etr2460 left a comment

Choose a reason for hiding this comment

graceguo-supercat commented Sep 4, 2019 •

edited

codecov-io commented Sep 7, 2019 •

edited

graceguo-supercat Sep 10, 2019 •

edited

graceguo-supercat Sep 10, 2019 •

edited

graceguo-supercat Sep 10, 2019 •

edited