feat(native-filters): Add default first value to select filter #13726

simcha90 · 2021-03-22T08:58:49Z

SUMMARY

This PR add option to choose by default first value for select filter plugin. This feature has more priority then default value, so if this feature enabled it will used instead of default value

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2021-03-22.at.10.57.00.mov

TEST PLAN

ADDITIONAL INFORMATION

villebro

One small remark

superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx

codecov · 2021-03-22T15:47:52Z

Codecov Report

Merging #13726 (cc0dc6f) into master (a4fd6b8) will decrease coverage by 0.39%.
The diff coverage is 75.75%.

❗ Current head cc0dc6f differs from pull request most recent head 4919889. Consider uploading reports for the commit 4919889 to get more accurate results

@@            Coverage Diff             @@
##           master   #13726      +/-   ##
==========================================
- Coverage   79.30%   78.91%   -0.40%     
==========================================
  Files         939      939              
  Lines       47541    47561      +20     
  Branches     5938     5949      +11     
==========================================
- Hits        37703    37531     -172     
- Misses       9717     9890     +173     
- Partials      121      140      +19

Flag	Coverage Δ
cypress	`51.72% <82.60%> (-2.52%)`	⬇️
javascript	`69.36% <3.84%> (-0.05%)`	⬇️

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

Impacted Files	Coverage Δ
...tend/src/filters/components/Select/controlPanel.ts	`18.18% <0.00%> (-1.82%)`	⬇️
superset/views/base.py	`76.47% <50.00%> (-0.40%)`	⬇️
...c/filters/components/Select/SelectFilterPlugin.tsx	`76.92% <77.27%> (-3.57%)`	⬇️
...tersConfigModal/FiltersConfigForm/DefaultValue.tsx	`100.00% <100.00%> (ø)`
...nd/src/filters/components/Select/transformProps.ts	`81.81% <100.00%> (ø)`
...et-frontend/src/filters/components/Select/types.ts	`100.00% <100.00%> (ø)`
superset/config.py	`90.68% <100.00%> (+0.03%)`	⬆️
superset/utils/async_query_manager.py	`81.37% <100.00%> (+0.37%)`	⬆️
...t-frontend/src/dashboard/containers/SliceAdder.jsx	`0.00% <0.00%> (-100.00%)`	⬇️
...c/dashboard/components/dnd/AddSliceDragPreview.jsx	`35.71% <0.00%> (-64.29%)`	⬇️
... 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 a4fd6b8...4919889. Read the comment docs.

junlin-qa

@simcha90 when the default checkbox is checked, user can not remove default select by clicking x.
Expected behavior: User should be able to remove default select anytime, and it clears the checkbox when value is removed. please address it before merging. thanks!

� Conflicts: � superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DefaultValue.tsx

superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx

This reverts commit 58b9199

github-actions · 2021-04-07T13:20:15Z

Ephemeral environment shutdown and build artifacts deleted.

� Conflicts: � superset-frontend/package-lock.json � superset-frontend/package.json � superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx

villebro

One comment about readability, other than that LGTM

villebro · 2021-04-11T07:34:19Z

superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx

+  const handleChange = (value?: SelectValue | number | string) => {
+    let selectValue: (number | string)[];
+    if (value === FIRST_VALUE) {
+      selectValue = forceFirstValue ? [] : firstItem;
+    } else {
+      selectValue = ensureIsArray<number | string>(value);
+    }
+    setValues(selectValue);

    const emptyFilter =
-      enableEmptyFilter && !inverseSelection && resultValue?.length === 0;
+      enableEmptyFilter && !inverseSelection && selectValue?.length === 0;
+
+    // Set currentState value for a case of non-`defaultToFirstItem`
+    const stateValue = selectValue.length ? selectValue : null;

    const dataMask = {
      extraFormData: getSelectExtraFormData(
        col,
-        resultValue,
+        selectValue,
        emptyFilter,
        inverseSelection,
      ),
      currentState: {
-        value: resultValue.length ? resultValue : null,
+        // We need to save in state `FIRST_VALUE` as some const and not as REAL value,
+        // because when FiltersBar check if all filters initialized it compares `defaultValue` with this value
+        // and because REAL value can be unpredictable for users that have different data for same dashboard we use `FIRST_VALUE`
+        value: value === FIRST_VALUE ? FIRST_VALUE : stateValue,


With the new logic this section is becoming slightly difficult to follow and could potentially be simplified. There's a few value === FIRST_VALUE checks, and the names value, selectValue, stateValue make it not obvious to the casual reader what these mean. I propose making these more explicit, e.g. renaming value to selectedValue (=what's the current selection) and constructing stateValue (=what goes in currentState) earlier on so we don't need to do a

currentState: { value: value === FIRST_VALUE ? FIRST_VALUE : stateValue }

later on. It would be nice if stateValue would already be fully ready by the time we arrive at that point, making the final currentState look more like this:

currentState: { value: stateValue }

I simplified logic can you look please one more time?

villebro

LGTM - Tested enabling and disabling on both filter tab and as a cross filter viz component. One minor observation: The emitted cross filter is now displayed as __FIRST_VALUE__.

However, this is a known limitation in the current cross filter indicator, and should be addressed in a separate pr.

* master: fix: unable to apply logging format (#14074) refactor: Bootstrap to AntD - Slider (#13989) chore(spa refactor): refactoring dashboard to use api's instead of bootstrapdata (#13306) fix(listview): update listview feature flag (#13906) Add docs for configuring Docker Compose setup (#13961) feat: invalid password error message (Postgres) (#14038) fix: flacky test in test_update_dataset_item_w_override_columns (#14082) feat: Implement Celery SoftTimeLimit handling (#13740) feat: only send alert error emails to owners of the alert (#13862) feat: add descriptions to report emails (#13827) Make chart exclude itself from cross filtering (#14046) fix: fix bug when remove chart not removing it's related cross filter data (#14081) feat(native-filters): Add default first value to select filter (#13726) feat: Make async query JWT cookie domain configurable (#14007) fix: add exception to catch session not having JWT (#14036) # Conflicts: # superset-frontend/src/dashboard/actions/hydrate.js # superset/views/core.py

* master: (53 commits) test: Adds tests to the UndoRedoKeyListeners component (#13919) chore: Adds dataMask reducer to reducerIndex (#13951) test: Tests audit for the Dashboard FilterBar (#13916) fix: unable to apply logging format (#14074) refactor: Bootstrap to AntD - Slider (#13989) chore(spa refactor): refactoring dashboard to use api's instead of bootstrapdata (#13306) fix(listview): update listview feature flag (#13906) Add docs for configuring Docker Compose setup (#13961) feat: invalid password error message (Postgres) (#14038) fix: flacky test in test_update_dataset_item_w_override_columns (#14082) feat: Implement Celery SoftTimeLimit handling (#13740) feat: only send alert error emails to owners of the alert (#13862) feat: add descriptions to report emails (#13827) Make chart exclude itself from cross filtering (#14046) fix: fix bug when remove chart not removing it's related cross filter data (#14081) feat(native-filters): Add default first value to select filter (#13726) feat: Make async query JWT cookie domain configurable (#14007) fix: add exception to catch session not having JWT (#14036) Use consistent chart value (#14031) fix: Use superset generic db to catch external_metadata queries (#13974) ...

…e#13726) * feat: native filters first default value * fix: fix CR notes * feat: add support for `Place` type * refactor: sync with master * feat: add first value to Select filters * refactor: fix CR notes * refactor: updates usage of `ownFilters` to `ownState` * Revert "refactor: updates usage of `ownFilters` to `ownState`" This reverts commit 58b9199 * fix: revert mocks * fix: fix CR notes * chore: update package json * chore: update package json * chore: update package json * test: fix tests * fix: fix BE empty metrics * lint: fix lint * fix: fix BE empty metrics * refactor: fix Cr notes

feat: native filters first default value

3eb7524

pull-request-size bot added the size/M label Mar 22, 2021

villebro reviewed Mar 22, 2021

View reviewed changes

superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx Outdated Show resolved Hide resolved

fix: fix CR notes

b070c1c

apache deleted a comment from github-actions bot Mar 22, 2021

junlincc added dashboard:native-filters Related to the native filters of the Dashboard revisit:design-sys labels Mar 24, 2021

junlin-qa reviewed Mar 24, 2021

View reviewed changes

simcha90 mentioned this pull request Apr 4, 2021

feat(native-filters): add AppSection type apache-superset/superset-ui#1038

Merged

simcha90 added 4 commits April 4, 2021 09:46

feat: add support for Place type

53f2a17

Merge branch 'master' of github.com:apache/superset into selec_first

ee84375

� Conflicts: � superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DefaultValue.tsx

refactor: sync with master

5da45bb

feat: add first value to Select filters

37b3f0e

pull-request-size bot added size/L and removed size/M labels Apr 5, 2021

refactor: fix CR notes

4d4b371

villebro reviewed Apr 6, 2021

View reviewed changes

simcha90 added 8 commits April 6, 2021 16:32

refactor: updates usage of ownFilters to ownState

58b9199

Revert "refactor: updates usage of ownFilters to ownState"

ce4b2aa

This reverts commit 58b9199

fix: revert mocks

22d7f7a

fix: fix CR notes

10cec8a

Merge branch 'master' of github.com:apache/superset into selec_first

21040f4

chore: update package json

5e0e87a

chore: update package json

10b9557

chore: update package json

de733f3

simcha90 closed this Apr 7, 2021

simcha90 reopened this Apr 7, 2021

Merge branch 'master' of github.com:apache/superset into selec_first

caed773

simcha90 added 5 commits April 8, 2021 13:06

test: fix tests

95baf89

fix: fix BE empty metrics

1c2f278

Merge branch 'master' of github.com:apache/superset into selec_first

8ccb6f0

� Conflicts: � superset-frontend/package-lock.json � superset-frontend/package.json � superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx

lint: fix lint

5f37308

fix: fix BE empty metrics

9db8487

amitmiran137 added the v1.2 label Apr 11, 2021

villebro reviewed Apr 11, 2021

View reviewed changes

refactor: fix Cr notes

4919889

villebro approved these changes Apr 12, 2021

View reviewed changes

villebro merged commit 468638c into apache:master Apr 12, 2021

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(native-filters): Add default first value to select filter #13726

feat(native-filters): Add default first value to select filter #13726

simcha90 commented Mar 22, 2021 •

edited by villebro

Loading

villebro left a comment

codecov bot commented Mar 22, 2021 •

edited

Loading

junlin-qa left a comment

github-actions bot commented Apr 7, 2021

villebro left a comment

villebro Apr 11, 2021

simcha90 Apr 12, 2021

villebro left a comment

feat(native-filters): Add default first value to select filter #13726

feat(native-filters): Add default first value to select filter #13726

Conversation

simcha90 commented Mar 22, 2021 • edited by villebro Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

villebro left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 22, 2021 • edited Loading

Codecov Report

junlin-qa left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 7, 2021

villebro left a comment

Choose a reason for hiding this comment

villebro Apr 11, 2021

Choose a reason for hiding this comment

simcha90 Apr 12, 2021

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

simcha90 commented Mar 22, 2021 •

edited by villebro

Loading

codecov bot commented Mar 22, 2021 •

edited

Loading