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(explore): new datasets have autocomplete filters enabled if UX_BETA is set #15864

Merged
merged 5 commits into from Jul 27, 2021

Conversation

kgabryje
Copy link
Member

@kgabryje kgabryje commented Jul 23, 2021

SUMMARY

If UX_BETA feature flag is set to True, all new datasets will have autocomplete filters enabled by default. The existing datasets will not be modified.
The existing datasets have been set to use autocompleting feature regardless of the feature flag value.
Also, adhoc filters in drag and drop mode have "IN" selected as a if UX_BETA is enabled.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2021-07-23.at.13.37.34.mov
Screen.Recording.2021-07-26.at.12.32.25.mov

TESTING INSTRUCTIONS

  1. Enable UX_BETA feature flag
  2. Create a new dataset and verify that it has "Autocomplete filters" enabled by default
  3. Create an adhoc filter and verify that after selecting column you can select a value from suggestions. Also, if you're using drag and drop, "IN" operator should be selected by default.

ADDITIONAL INFORMATION

CC @junlincc @graceguo-supercat

@codecov
Copy link

codecov bot commented Jul 23, 2021

Codecov Report

Merging #15864 (d45366d) into master (9c81599) will decrease coverage by 0.24%.
The diff coverage is 35.29%.

❗ Current head d45366d differs from pull request most recent head 07994b6. Consider uploading reports for the commit 07994b6 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #15864      +/-   ##
==========================================
- Coverage   77.08%   76.84%   -0.25%     
==========================================
  Files         984      984              
  Lines       51788    51801      +13     
  Branches     7037     7038       +1     
==========================================
- Hits        39923    39807     -116     
- Misses      11640    11769     +129     
  Partials      225      225              
Flag Coverage Δ
hive ?
javascript 71.73% <66.66%> (-0.01%) ⬇️
presto ?

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

Impacted Files Coverage Δ
...l/AdhocFilterEditPopoverSimpleTabContent/index.tsx 65.68% <ø> (ø)
superset/examples/bart_lines.py 27.58% <0.00%> (-0.99%) ⬇️
superset/examples/country_map.py 25.00% <0.00%> (-0.65%) ⬇️
superset/examples/energy.py 26.19% <0.00%> (-0.64%) ⬇️
superset/examples/flights.py 17.64% <0.00%> (-0.54%) ⬇️
superset/examples/long_lat.py 23.80% <0.00%> (-0.59%) ⬇️
superset/examples/multiformat_time_series.py 16.66% <0.00%> (-0.36%) ⬇️
superset/examples/paris.py 26.92% <0.00%> (-1.08%) ⬇️
superset/examples/random_time_series.py 19.44% <0.00%> (-0.56%) ⬇️
superset/examples/sf_population_polygons.py 26.92% <0.00%> (-1.08%) ⬇️
... and 10 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 9c81599...07994b6. Read the comment docs.

@kgabryje
Copy link
Member Author

/testenv up FEATURE_UX_BETA=true

@github-actions
Copy link
Contributor

@kgabryje Ephemeral environment spinning up at http://34.213.99.235:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@kgabryje
Copy link
Member Author

/testenv up FEATURE_UX_BETA=true

@github-actions
Copy link
Contributor

@kgabryje Ephemeral environment spinning up at http://54.244.217.223:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@junlincc junlincc requested review from michael-s-molina and villebro and removed request for ktmud and etr2460 July 26, 2021 22:44
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 - one minor 🤔 but it shouldn't be blocking as this is behind a FF

Comment on lines +317 to +320
if (isFeatureEnabled(FeatureFlag.UX_BETA)) {
config.operator = OPERATOR_ENUM_TO_OPERATOR_TYPE[Operators.IN].operation;
config.operatorId = Operators.IN;
}
Copy link
Member

Choose a reason for hiding this comment

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

When we drop a non-string column, the UX feels slightly unintuitive (here I dropped the num column):

image

Since we have the full set of columns along with their type_generic, maybe we could consider having different behavior for numeric and temporal column types while we're at it?

Copy link
Member

Choose a reason for hiding this comment

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

good idea! it would be great to have the numeric slider in adhoc filter eventually when we redesign the entire component.
For temporal columns, i doubt users would use adhoc filter for it.
For numeric, we can use "=" or "<"? either way, the preselection is not gonna trigger any query, or adding additional clicks to selection, so the change is harmless. it really depends on what our best guess is without further data.

@kgabryje kgabryje merged commit 3f5237a into apache:master Jul 27, 2021
@kgabryje
Copy link
Member Author

kgabryje commented Jul 27, 2021

Ville's comment will be addressed in a follow up PR

@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

opus-42 pushed a commit to opus-42/incubator-superset that referenced this pull request Nov 14, 2021
…ETA is set (apache#15864)

* feat(explore): new datasets have autocomplete filters enabled if UX_BETA is set

* Lint fix

* Set autocomplete to True in example datasets

* Use "IN" as a default operator in dnd filters

* Default operator only if UX_BETA enabled
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
…ETA is set (apache#15864)

* feat(explore): new datasets have autocomplete filters enabled if UX_BETA is set

* Lint fix

* Set autocomplete to True in example datasets

* Use "IN" as a default operator in dnd filters

* Default operator only if UX_BETA enabled
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
…ETA is set (apache#15864)

* feat(explore): new datasets have autocomplete filters enabled if UX_BETA is set

* Lint fix

* Set autocomplete to True in example datasets

* Use "IN" as a default operator in dnd filters

* Default operator only if UX_BETA enabled
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
…ETA is set (apache#15864)

* feat(explore): new datasets have autocomplete filters enabled if UX_BETA is set

* Lint fix

* Set autocomplete to True in example datasets

* Use "IN" as a default operator in dnd filters

* Default operator only if UX_BETA enabled
@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 13, 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/M 🚢 1.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Upcoming change]Enable Autocomplete filter in Edit Dataset setting by default
4 participants