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): Hide time filters if loaded datasets don't have temporal columns #15225

Merged
merged 5 commits into from
Jun 17, 2021

Conversation

kgabryje
Copy link
Member

@kgabryje kgabryje commented Jun 17, 2021

SUMMARY

If none of the datasets used by charts on current dashboard have any temporal columns, there's no point in applying Time range, Time column or Time grain filters to them. This PR implements disables selecting those filter types on dashboards without datasets with temporal columns.
Also, there's no point in pre-filtering by time range datasets without temporal columns. This PR implements hiding time range picker pre-filter if current dataset doesn't have temporal columns.
I also removed "px" suffix from "0px" css values to remove linter warnings.

Second part of the related issue (#15068) was excluding charts that use dataset without temporal columns from scope when creating a time filter. We've decided to handle this as a separate issue (#14977). CC @junlincc

Rejected alternatives: hiding time filters instead of disabling options in select button. We decided that it might be confusing for the users if some filter types just "disappeared".

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before: see #15068

After:
Case 1: only datasets without temporal columns - time filters disabled, time range pre-filter hidden
https://user-images.githubusercontent.com/15073128/122394612-7589fc80-cf76-11eb-9af4-f3662b71d432.mov

Case 2: only some datasets have temporal columns - time filters enabled, time range pre-filter hidden if a dataset without temporal columns is selected ("threads" dataset has temporal columns, "members_channels_2" doesn't)
https://user-images.githubusercontent.com/15073128/122388521-3eb0e800-cf70-11eb-96b6-ad5f2651bf69.mov

TESTING INSTRUCTIONS

  1. Enable DASHBOARD_NATIVE_FILTERS feature flag
  2. Create a dashboard using datasets without temporal columns (e.g. covid_vaccines from test data)
  3. Open native filters modal, verify that only Value and Numerical Range filter types are enabled.
  4. Create a dashboard using multiple datasets, some of which have temporal columns, and some don't
  5. Open native filters modal, verify that all 5 filter types are available
  6. Select a dataset with temporal columns and check Pre-filter available values. Verify that time range filter shows up
  7. Select a dataset without temporal columns and go to pre-filtering section. Verify that time range filter doesn't show up.

ADDITIONAL INFORMATION

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.

LGTM - code looks good and works as expected in testing 👍

@michael-s-molina
Copy link
Member

@kgabryje I don't need to remove the px from the zeros 😆. The CSS minifiers will do that anyway.

@kgabryje
Copy link
Member Author

kgabryje commented Jun 17, 2021

@kgabryje I don't need to remove the px from the zeros 😆. The CSS minifiers will do that anyway.

I know, but Webstorm was displaying lots of warnings 😄
image

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

LGTM. Nice idea!

@michael-s-molina
Copy link
Member

@kgabryje I don't need to remove the px from the zeros 😆. The CSS minifiers will do that anyway.

I know, but Webstorm was displaying lots of warnings 😄
image

Oh, gotcha! I hate warnings too 🤣

@codecov
Copy link

codecov bot commented Jun 17, 2021

Codecov Report

Merging #15225 (a826630) into master (e5187a4) will increase coverage by 0.09%.
The diff coverage is 84.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #15225      +/-   ##
==========================================
+ Coverage   77.08%   77.18%   +0.09%     
==========================================
  Files         971      971              
  Lines       50242    50308      +66     
  Branches     6498     6140     -358     
==========================================
+ Hits        38731    38828      +97     
+ Misses      11306    11276      -30     
+ Partials      205      204       -1     
Flag Coverage Δ
javascript 71.80% <84.37%> (+0.20%) ⬆️

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

Impacted Files Coverage Δ
...omponents/nativeFilters/FilterBar/Header/index.tsx 91.89% <0.00%> (ø)
...onfigModal/FiltersConfigForm/FiltersConfigForm.tsx 69.15% <85.18%> (+0.94%) ⬆️
...ters/FiltersConfigModal/FiltersConfigForm/utils.ts 93.75% <100.00%> (+0.41%) ⬆️
superset-frontend/src/dataMask/reducer.ts 71.42% <100.00%> (+6.84%) ⬆️
...nfigModal/FiltersConfigForm/CollapsibleControl.tsx 91.30% <0.00%> (-8.70%) ⬇️
...ters/FiltersConfigModal/FiltersConfigForm/state.ts 96.66% <0.00%> (-3.34%) ⬇️
superset-frontend/src/views/CRUD/hooks.ts 53.02% <0.00%> (ø)
...end/src/SqlLab/components/RunQueryActionButton.tsx 64.28% <0.00%> (ø)
...hboard/components/nativeFilters/FilterBar/utils.ts 75.00% <0.00%> (ø)
... 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 fe5381d...a826630. Read the comment docs.

@kgabryje kgabryje merged commit c7c6375 into apache:master Jun 17, 2021
@graceguo-supercat
Copy link

graceguo-supercat commented Jun 21, 2021

@kgabryje i am scanning dashboards with current master branch.
this PR cause many of our dashboards can not add time filter. I am pretty sure those dataset has temporal column.

I am not sure what is logic you decide a dataset has "temporal" column, many of our users can check Is Temporal from dataset editor:
Screen Shot 2021-06-21 at 2 36 37 PM

Could you investigate and test it?

@villebro
Copy link
Member

@graceguo-supercat This PR relies on backend functionality added in this PR: #15188 . I can investigate this.

@villebro
Copy link
Member

I am unable to reproduce this problem. I created a datasource with the following columns:
image

When I check the available datatypes in for the datasource in the Dashboard view in Redux, they show up correctly (check column_types, where 0 == GenericDataType.NUMERIC, 1 == GenericDataType.STRING, 2 == GenericDataType.TEMPORAL, 3 == GenericDataType.BOOLEAN):
image

When I attempt to add a temporal filter, it works as expected:
image

When I uncheck "is temporal", the temporal datatype is removed from Redux, and consequently the temporal filter types are also removed from the filter config modal as expected:
image

image

image

@kgabryje
Copy link
Member Author

@graceguo-supercat I tested on a few dashboards and I can't reproduce the issue.
Case 1: a dashboard with multiple datasets, some of them have temporal columns and some don't (Slack Dashboard from test data) - time filters are available.

Case 2: a dashboard with 1 dataset with temporal columns (World Bank's Data), time filters are available

Case 3: a dashboard with a dataset which doesn't have temporal columns (Covid Vaccine Dashboard), time filters are unavailable.

Case 4: Covid Vaccine Dashboard with manually added temporal column through Edit Dataset modal, time filters are available.

cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
… temporal columns (apache#15225)

* feat(native-filters): Hide time filters if loaded datasets don't have temporal columns

* Remove "px" suffixes to fix warnings

* Disable an option instead of hiding filter types

* Fix tests

* Add 2 more tests
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
… temporal columns (apache#15225)

* feat(native-filters): Hide time filters if loaded datasets don't have temporal columns

* Remove "px" suffixes to fix warnings

* Disable an option instead of hiding filter types

* Fix tests

* Add 2 more tests
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
… temporal columns (apache#15225)

* feat(native-filters): Hide time filters if loaded datasets don't have temporal columns

* Remove "px" suffixes to fix warnings

* Disable an option instead of hiding filter types

* Fix tests

* Add 2 more tests
@mistercrunch 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 1.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[native-filters]hide time related filters when there's no temporal column in datasets
5 participants