gabotorresruiz commented on code in PR #37260:
URL: https://github.com/apache/superset/pull/37260#discussion_r2728925809
##########
superset-frontend/src/dashboard/components/nativeFilters/selectors.ts:
##########
@@ -281,8 +285,19 @@ const getStatus = ({
}
if (column && rejectedColumns?.has(column))
return IndicatorStatus.Incompatible;
- if (column && appliedColumns?.has(column) && hasValue) {
- return APPLIED_STATUS;
+ // If filter has a value and column is not rejected, show as applied
+ // This works even when chart hasn't loaded yet (appliedColumns is empty)
+ // The dataMask state is the source of truth for filter application
+ if (column && hasValue && !rejectedColumns?.has(column)) {
Review Comment:
Nit: The `rejectedColumns?.has(column)` on line `286` already returns
`Incompatible` for rejected columns, so if we reach line 291 the column is
guaranteed not rejected.
This could be simplified to:
```js
if (column && hasValue) {
...
}
```
Not blocking, just a readability improvement.
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx:
##########
@@ -416,11 +416,18 @@ const FilterBar: FC<FiltersBarProps> = ({
const handleClearAll = useCallback(() => {
const newClearAllTriggers = { ...clearAllTriggers };
nativeFilterValues.forEach(filter => {
- const { id } = filter;
+ const { id, filterType } = filter;
+ // Range filters use [null, null] as the cleared value; others use null
+ const clearedValue = filterType === 'filter_range' ? [null, null] : null;
+ const clearedDataMask = {
+ filterState: { value: clearedValue },
+ extraFormData: {},
+ };
if (dataMaskSelected[id]) {
+ dispatch(updateDataMask(id, clearedDataMask));
setDataMaskSelected(draft => {
if (draft[id].filterState?.value !== undefined) {
- draft[id].filterState!.value = undefined;
+ draft[id].filterState!.value = clearedValue;
Review Comment:
This assignment can no longer be `undefined` (as it could be previously),
since `clearedValue` is now always defined as:
```js
const clearedValue = filterType === 'filter_range' ? [null, null] : null;
```
Are we certain this change does not impact any existing checks or behavior
elsewhere in the codebase that rely on `undefined` specifically?
##########
superset-frontend/src/dashboard/components/nativeFilters/selectors.ts:
##########
@@ -281,8 +285,19 @@ const getStatus = ({
}
if (column && rejectedColumns?.has(column))
return IndicatorStatus.Incompatible;
- if (column && appliedColumns?.has(column) && hasValue) {
- return APPLIED_STATUS;
+ // If filter has a value and column is not rejected, show as applied
+ // This works even when chart hasn't loaded yet (appliedColumns is empty)
+ // The dataMask state is the source of truth for filter application
+ if (column && hasValue && !rejectedColumns?.has(column)) {
+ // If chart has loaded and confirmed this column was applied, use that
+ if (appliedColumns?.has(column)) {
+ return APPLIED_STATUS;
+ }
+ // If chart hasn't loaded yet but we have a value, assume it's applied
+ // This allows showing filter indicators before query response is available
+ if (!appliedColumns || appliedColumns.size === 0) {
Review Comment:
So if a chart hasn't loaded yet (`appliedColumns` is empty), the indicator
will show as `"Applied"`?
Would it make sense to add a loading/pending state instead of assuming
`"Applied"`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
