msyavuz commented on code in PR #37012:
URL: https://github.com/apache/superset/pull/37012#discussion_r2677021947
##########
superset-frontend/src/dashboard/components/nativeFilters/state.ts:
##########
Review Comment:
This needs tests at any case
##########
superset-frontend/src/dashboard/components/nativeFilters/state.ts:
##########
Review Comment:
Are changes in this file needed for the fix? This could break in a lot of
ways. Tagging @kgabryje for a look as well
##########
superset-frontend/src/dashboard/components/nativeFilters/state.ts:
##########
@@ -144,20 +144,38 @@ export function useSelectFiltersInScope(filters: (Filter
| Divider)[]) {
let filtersInScope: (Filter | Divider)[] = [];
const filtersOutOfScope: (Filter | Divider)[] = [];
- // we check native filters scopes only on dashboards with tabs
- if (!dashboardHasTabs) {
- filtersInScope = filters;
- } else {
- filters.forEach(filter => {
+ filters.forEach(filter => {
+ // Dividers are always in scope
+ if (isFilterDivider(filter)) {
+ filtersInScope.push(filter);
+ return;
+ }
+
+ // If dashboard has tabs, check scope based on visibility
+ if (dashboardHasTabs) {
const filterInScope = isFilterInScope(filter);
-
if (filterInScope) {
filtersInScope.push(filter);
} else {
filtersOutOfScope.push(filter);
}
- });
- }
+ } else {
+ // If no tabs, check if filter has any charts in scope
+ // Only mark as out of scope if chartsInScope is explicitly defined
AND empty
+ const hasExplicitChartsInScope = Array.isArray(filter.chartsInScope);
+ const hasChartsInScope =
+ hasExplicitChartsInScope && filter.chartsInScope!.length > 0;
Review Comment:
This is named in reverse right? We are checking which filters are in scope,
not which charts would be effected.
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx:
##########
@@ -254,6 +254,14 @@ const FilterControls: FC<FilterControlsProps> = ({
/>
)}
+ {!showCollapsePanel && filtersOutOfScope.length > 0 && (
Review Comment:
Can we add this check to the already existing display conditions for
`FiltersOutOfScopeCollapsible` Wouldn't this show two collapsibles?
--
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]
