-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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 search all filter options #14710
Conversation
6f2b3c3
to
3998f22
Compare
e6727a5
to
604100f
Compare
Codecov Report
@@ Coverage Diff @@
## master #14710 +/- ##
==========================================
- Coverage 77.32% 77.30% -0.02%
==========================================
Files 962 962
Lines 49093 49147 +54
Branches 6165 6182 +17
==========================================
+ Hits 37959 37994 +35
- Misses 10930 10949 +19
Partials 204 204
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
type DataMaskAction = | ||
| { type: 'ownState'; ownState: JsonObject } | ||
| { | ||
type: 'filterState'; | ||
extraFormData: ExtraFormData; | ||
filterState: { value: SelectValue }; | ||
}; | ||
|
||
function reducer(state: DataMask, action: DataMaskAction): DataMask { | ||
switch (action.type) { | ||
case 'ownState': | ||
return { | ||
...state, | ||
ownState: { | ||
...(state.ownState || {}), | ||
...action.ownState, | ||
}, | ||
}; | ||
case 'filterState': | ||
return { | ||
...state, | ||
extraFormData: action.extraFormData, | ||
filterState: { | ||
...(state.filterState || {}), | ||
...action.filterState, | ||
}, | ||
}; | ||
default: | ||
return { | ||
...state, | ||
}; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to migrate from multiple useState
s and useEffects
to useReducer
, as the full dataMask
will be updated by different actions but needs to be submitted as a whole to setDataMask
. Therefore, useReducer
seemed like a cleaner approach.
const initSelectValue: SelectValue = | ||
// `defaultValue` can be `FIRST_VALUE` if `defaultToFirstItem` is checked, so need convert it to correct value for Select | ||
defaultValue === FIRST_VALUE ? [] : defaultValue ?? []; | ||
|
||
const firstItem: SelectValue = data[0] | ||
? (groupby.map(col => data[0][col]) as string[]) ?? initSelectValue | ||
: initSelectValue; | ||
|
||
// If we are in config modal we always need show empty select for `defaultToFirstItem` | ||
const [values, setValues] = useState<SelectValue>( | ||
defaultToFirstItem && appSection !== AppSection.FILTER_CONFIG_MODAL | ||
? firstItem | ||
: initSelectValue, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The FIRST_VALUE
placeholder doesn't really work, as we don't know what the first value is for each individual user, which means that it won't be possible to persist extraFormData
at filter configuration time.
@villebro I noticed two things:
20210522102235847.mp4 |
@michael-s-molina this is weird - the "No Results" behavior you describe should not happen after bumping the The search problem you faced is due to previously using the |
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx
Outdated
Show resolved
Hide resolved
@villebro After pulling the changes and bumping the |
// handle changes coming from application, e.g. "Clear all" button | ||
if (JSON.stringify(values) !== JSON.stringify(filterState.value)) { | ||
handleChange(filterState.value); | ||
} | ||
}, [JSON.stringify(filterState.value)]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be removed in a follow-up PR when all state handling is moved to the application
import { Select } from 'src/common/components'; | ||
import { FIRST_VALUE, PluginFilterSelectProps, SelectValue } from './types'; | ||
import { debounce } from 'lodash'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently it's better to import debounce from 'lodash/debounce'
, as it results in smaller bundle size (and we probably should apply that in other places in superset).
References:
https://stackoverflow.com/questions/35250500/correct-way-to-import-lodash
https://www.blazemeter.com/blog/the-correct-way-to-import-lodash-libraries-a-benchmark
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know 👍
row_limit: 10000, | ||
row_limit: 1000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default for Filter Box is 1000 rows.
* master: (163 commits) fix(native-filters): Manage default value of filters by superset (apache#14785) fix: Additional ResultSet tests (apache#14741) chore: added BasicParametersMixin to Redshift (apache#14752) fix: make dataset list sort case insensitive (apache#14528) fix: use encodeURIComponent when getting table metadata (apache#14790) fix: ensure engine is outside parameters (apache#14787) database modal should close on connect with tab layout (apache#14771) feat(native-filters): add search all filter options (apache#14710) fix: extra query in Dashboard when native filter enabled (apache#14770) chore: Improves the native filters UI/UX - iteration 2 (apache#14753) fix(native filters): Fix explore state (apache#14779) fix(explore): DndColumnSelect not handling controls with "multi: false" (apache#14737) feat: Create BigQuery Parameters for DatabaseModal (apache#14721) feat: enable user impersonation in GSheets (apache#14767) fix: add DB should not say it's Postgres (apache#14766) Revert "fix(dashboard): multiple query trigger when native filter enabled (apache#14734)" (apache#14762) feat: save database with new dynamic form (apache#14583) fix: save non-parameter DBs (apache#14759) chore: Removes ColorSchemeControl.less (apache#14199) fix(explore): Icons width (apache#14717) ...
* feat(native-filters): add search all filter options * add tests * fix default value * implement ILIKE operator * rebump packages * fix test * address comments * fix state changes coming from application * fix debouncer
* feat(native-filters): add search all filter options * add tests * fix default value * implement ILIKE operator * rebump packages * fix test * address comments * fix state changes coming from application * fix debouncer
* feat(native-filters): add search all filter options * add tests * fix default value * implement ILIKE operator * rebump packages * fix test * address comments * fix state changes coming from application * fix debouncer
SUMMARY
Add support for "Search all filter options" option to native select filter to obtain feature parity with same functionality in Filter Box. Other changes:
lower(${key}) like '%${input}%'
, which only worked on those backends that support thelower
function (a non-ANSI SQL function). This has been replaced by the following logicILIKE
operator ( https://docs.sqlalchemy.org/en/13/core/sqlelement.html#sqlalchemy.sql.expression.ColumnElement.ilike ). This makes it possible to utilize theILIKE
operator where supported, e.g. on Postgres.>=
operator. Previously the Filter Box would fail silently for numeric columns, as it tried to calllower()
on the numeric column, which usually doesn't work.ILIKE
operator option to adhoc filter popover (see screenshot)SCREENSHOT
See below how searching for entries that don't fit into the first 1000 entries are found when the feature is enabled:
https://user-images.githubusercontent.com/33317356/119319233-d88fb880-bc82-11eb-9a11-e63aca9e243f.mp4
Also add case case insensitive LIKE operator to adhoc filter popover to support
ILIKE
SqlAlchemy operator:Also pull in the following PRs from
superset-ui
:TEST PLAN
ADDITIONAL INFORMATION