alexandrusoare commented on code in PR #35683:
URL: https://github.com/apache/superset/pull/35683#discussion_r2545544886
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/components/CustomHeader.tsx:
##########
@@ -102,18 +167,44 @@ const CustomHeader: React.FC<CustomHeaderParams> = ({
else clearSort();
};
- const handleFilterClick = async (e: React.MouseEvent) => {
- e.stopPropagation();
+ const handleFilterClick = async (e?: React.MouseEvent) => {
+ if (e) {
+ e.stopPropagation();
+ }
setFilterVisible(!isFilterVisible);
- const filterInstance = await api.getColumnFilterInstance<any>(column);
+ const filterInstance = (await api.getColumnFilterInstance(
+ column,
+ )) as AGGridFilterInstance | null;
const filterEl = filterInstance?.eGui;
if (filterEl && filterRef.current) {
- filterRef.current.innerHTML = '';
+ // Clear children safely without innerHTML to prevent XSS
+ while (filterRef.current.firstChild) {
+ filterRef.current.removeChild(filterRef.current.firstChild);
+ }
filterRef.current.appendChild(filterEl);
}
};
+ // Auto-open filter popover for the last filtered column
+ useEffect(() => {
+ if (lastFilteredColumn === colId && !isFilterVisible) {
+ const timeoutId = setTimeout(
+ () =>
+ autoOpenFilterAndFocus(
+ column,
+ api,
+ filterRef,
+ setFilterVisible,
+ lastFilteredInputPosition,
+ ),
+ FILTER_POPOVER_OPEN_DELAY,
+ );
Review Comment:
Why is there a need for delay here?
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getInitialFilterModel.ts:
##########
@@ -0,0 +1,54 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { isEmpty } from 'lodash';
+import type { AgGridChartState } from '@superset-ui/core';
+
+/**
+ * Determines the initial filter model for AG Grid
+ * Priority: chartState.filterModel > serverPaginationData.agGridFilterModel
+ *
+ * @param chartState - Saved chart state from permalink (can be partial)
+ * @param serverPaginationData - Server pagination data containing filter model
+ * @param serverPagination - Whether server pagination is enabled
+ * @returns Filter model object or undefined if no valid filter exists
+ */
+const getInitialFilterModel = (
+ chartState?: Partial<AgGridChartState>,
+ serverPaginationData?: Record<string, any>,
+ serverPagination?: boolean,
+): Record<string, any> | undefined => {
+ // Use chartState.filterModel if it exists and is not empty
+ const chartStateFilterModel =
+ chartState?.filterModel && !isEmpty(chartState.filterModel)
+ ? chartState.filterModel
+ : undefined;
+
+ // Use serverPaginationData.agGridFilterModel if server pagination is
enabled and it's not empty
+ const serverFilterModel =
+ serverPagination &&
+ serverPaginationData?.agGridFilterModel &&
+ !isEmpty(serverPaginationData.agGridFilterModel)
+ ? serverPaginationData.agGridFilterModel
+ : undefined;
+
Review Comment:
Let me make sure I understand this correcty. For example if you have some
filters applied to the aggridtable and you share a permalink, the filter is
going to be applied frontendside only right? And this is only for when
initiating the chart a.k.a opening a dashboard for example right? When you
apply another filter to the chart everything is going to become serverside?
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/useColDefs.ts:
##########
@@ -222,7 +222,7 @@ export const useColDefs = ({
}),
...(dataType === GenericDataType.Temporal && {
filterParams: {
- comparator: dateFilterComparator,
+ comparator: serverPagination ? () => 0 : dateFilterComparator,
Review Comment:
I understand that you needed to do something like this to prevent the client
side filtering to happen but seems a bit hacky, is there a better to handle
this?
##########
superset-frontend/src/components/Chart/ChartRenderer.jsx:
##########
@@ -357,9 +357,17 @@ class ChartRenderer extends Component {
?.behaviors.find(behavior => behavior === Behavior.DrillToDetail)
? { inContextMenu: this.state.inContextMenu }
: {};
- // By pass no result component when server pagination is enabled & the
table has a backend search query
+ // By pass no result component when server pagination is enabled & the
table has:
+ // - a backend search query, OR
+ // - non-empty AG Grid filter model
+ const hasSearchText = (ownState?.searchText?.length || 0) > 0;
+ const hasAgGridFilters =
+ ownState?.agGridFilterModel &&
Review Comment:
I think `FilterModel` would be a better fit than `agGridFilterModel`. What
do you think?
--
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]
