Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(native-filters): Add defaultValue for Native filters modal #12199

Merged
merged 23 commits into from
Feb 2, 2021

Conversation

simcha90
Copy link
Contributor

@simcha90 simcha90 commented Dec 24, 2020

SUMMARY

Add support for defaultValue in Native Filters Config Modal per filter type.

Changes:

  1. Native Filters Config Modal
  • added Select to choose filter type (now supports only Select filter and Range filter)
  • added SuperChart that renders appropriate Native filter as default value selector
  • added defaultValueQueries to keep updated queriesData that will passed to SuperChart
  • added defaultValueFormData to keep updated form data that will passed SuperChart
  • dataset now managed by Ant forms
  1. Select Native filter
  • now works only with array default values
  1. Filter Bar
  • added currentState to nativeFilters.filtersState in redux to correct save and update defaultValues for resetAll case and others
  • setExtraFormData hook upgraded to pass currentState

Flows:

  1. In Filters Config Modal
  • When change dataset -> reset column field -> reset default values
  • When change column field -> reset default values
  • When change filter type -> reset default values
  • When clicks allows multiple values -> reset default values
  1. In Filter Bar
  • When clicks Reset all should return to default values
  • When update some default value in Filters Config Modal, it should update also current value in Filter Bar

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2021-01-28.at.17.12.04.mov

TEST PLAN

  • Check above flows
  • No need check default values for Range filter (it should be fixed in other PR)

ADDITIONAL INFORMATION

@junlincc junlincc added the assigned:nielsen Assigned to the nielsen team label Dec 24, 2020
� Conflicts:
�	superset-frontend/spec/javascripts/dashboard/components/nativeFilters/ScopingTree_spec.tsx
�	superset-frontend/src/dashboard/components/nativeFilters/FilterBar.tsx
�	superset-frontend/src/dashboard/components/nativeFilters/FilterConfigForm.tsx
�	superset-frontend/src/dashboard/components/nativeFilters/FilterScope.tsx
�	superset-frontend/src/dashboard/components/nativeFilters/ScopingTree.tsx
�	superset-frontend/src/dashboard/components/nativeFilters/state.ts
�	superset-frontend/src/dashboard/components/nativeFilters/utils.ts
� Conflicts:
�	superset-frontend/src/dashboard/components/nativeFilters/FilterConfigForm.tsx
�	superset-frontend/src/dashboard/components/nativeFilters/FilterConfigModal.tsx
@junlincc
Copy link
Member

closes #12713

@junlincc junlincc added the dashboard:native-filters Related to the native filters of the Dashboard label Jan 28, 2021
@simcha90 simcha90 reopened this Jan 28, 2021
@codecov-io
Copy link

codecov-io commented Jan 28, 2021

Codecov Report

Merging #12199 (10529e9) into master (2b9d579) will decrease coverage by 1.35%.
The diff coverage is 39.24%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12199      +/-   ##
==========================================
- Coverage   63.23%   61.88%   -1.36%     
==========================================
  Files        1021      535     -486     
  Lines       50145    20037   -30108     
  Branches     5204     5242      +38     
==========================================
- Hits        31708    12399   -19309     
+ Misses      18222     7424   -10798     
+ Partials      215      214       -1     
Flag Coverage Δ
javascript 61.88% <39.24%> (-0.06%) ⬇️
python ?

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

Impacted Files Coverage Δ
...et-frontend/src/dashboard/actions/nativeFilters.ts 34.78% <0.00%> (ø)
...ard/components/nativeFilters/FilterConfigModal.tsx 69.74% <ø> (ø)
...t-frontend/src/dashboard/reducers/nativeFilters.ts 34.78% <ø> (ø)
...src/filters/components/Select/AntdSelectFilter.tsx 0.00% <0.00%> (ø)
...et-frontend/src/filters/components/Select/types.ts 0.00% <ø> (ø)
superset-frontend/src/filters/utils.ts 88.88% <ø> (ø)
...hboard/components/nativeFilters/CascadePopover.tsx 19.11% <7.69%> (+0.93%) ⬆️
...c/dashboard/components/nativeFilters/FilterBar.tsx 51.36% <30.00%> (+3.29%) ⬆️
...nd/src/dashboard/components/nativeFilters/state.ts 59.09% <36.73%> (-6.95%) ⬇️
...oard/components/nativeFilters/FilterConfigForm.tsx 70.00% <55.55%> (-18.89%) ⬇️
... and 514 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 2b9d579...10529e9. Read the comment docs.

@simcha90 simcha90 changed the title WIP: Add defaultValue for Native filters feat(native-filters): Add defaultValue for Native filters Jan 28, 2021
@simcha90 simcha90 changed the title feat(native-filters): Add defaultValue for Native filters feat(native-filters): Add defaultValue for Native filters modal Jan 28, 2021
@simcha90
Copy link
Contributor Author

simcha90 commented Jan 28, 2021

@agatapst @junlincc @villebro I synced this PR with master and updated it, so I think it's ready to review, if you think that this PR relevant for settings defaultValue in native filters

@junlincc junlincc requested review from villebro and suddjian January 28, 2021 21:52
@junlincc junlincc self-requested a review January 28, 2021 21:55
Copy link
Member

@junlincc junlincc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Simcha for the PR! a few issues found in this branch but not in master

  1. when i open the dashboard modal, screen below is what i see. have to click "+" to expand

Screen Shot 2021-01-28 at 2 40 00 PM

  1. reset button doesn't work
  2. after selecting filter type, default select dropdown does not show. "No Results" but there actually are options

Screen Shot 2021-01-28 at 2 44 25 PM

@simcha90 you have rebased right?

@junlincc
Copy link
Member

junlincc commented Jan 28, 2021

also from the UX standpoint, filter values select probably more than most of the time. @simcha90 @agatapst I think we should check allow multiple-selections box checked by default in the modal, for both default value setting and filter value select

� Conflicts:
�	superset-frontend/src/dashboard/components/nativeFilters/FilterBar.tsx
@simcha90
Copy link
Contributor Author

@junlincc Hi fixed your notes (about 3. it works for me, but may it was fixed by my other changes).
I also needed update some part of code to cause Reset All work, so I also updated description of PR with all changes

About multiple values, it can be good idea but I think in other PR in order to not increase size of this one, because implementing of this feature caused some cascading changes in other parts of code, so it have now XL size :)

cc @villebro

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass comments.

superset-frontend/src/dashboard/actions/nativeFilters.ts Outdated Show resolved Hide resolved
superset-frontend/src/dashboard/actions/nativeFilters.ts Outdated Show resolved Hide resolved
dataset: {
value: number;
label: string;
};
column: string;
defaultValue: string;
defaultValue: any;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be CurrentFilterState?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

explained in bottom comment 👇 :)

Comment on lines +85 to +86
defaultValue: any;
currentValue?: any;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Copy link
Contributor Author

@simcha90 simcha90 Feb 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's value that we are passing to the native filter, so it should be currentValue together with defaultValue and not currentState according current apis

I think may be we need to discuss in future if we want to render some things like Allow multiple selection checkbox inside of native filter chart so it should be really changed to currentState but for now all filters working only with currentValue

superset-frontend/src/filters/components/Select/types.ts Outdated Show resolved Hide resolved
@villebro villebro merged commit 465d986 into apache:master Feb 2, 2021
amitmiran137 pushed a commit to nielsen-oss/superset that referenced this pull request Feb 3, 2021
* master: (23 commits)
  feat(explore): clear search on dataset change (apache#12909)
  chore: remove SIP-38 feature flag (apache#12894)
  fix: Config for dataset health check (apache#12906)
  fix(chart): allow null for most query object props (apache#12905)
  feat: add separate endpoint to fetch function names for autocomplete (apache#12840)
  chore: add required review on master (apache#12694)
  fix: comment typo (apache#12898)
  Migrates Radio component from Bootstrap to AntD. (apache#12738)
  fix: allow users to reset their passwords (apache#12886)
  fix(explore): missing select when groupby without metrics (apache#12890)
  refactor: dbapi exception mapping for dbapi's (apache#12869)
  feat(style-theme): add support for custom superset themes (apache#12858)
  chore(lint): fix pre-commit error (apache#12884)
  refactor(color-schemes): refactor setting of color schemes (apache#12857)
  feat(native-filters): Add defaultValue for Native filters modal (apache#12199)
  feat(release): add github token to changelog script (apache#12872)
  fix(menu): always show settings dropdown (apache#12877)
  Migrates Label component from Bootstrap to AntD. (apache#12774)
  [Helm] Automate datasource import (apache#10771)
  build: Skip loading example data from configs in CI (apache#12610)
  ...
@mistercrunch 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assigned:nielsen Assigned to the nielsen team 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels dashboard:native-filters Related to the native filters of the Dashboard size/XL 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants