EnxDev commented on code in PR #44907:
URL: https://github.com/apache/superset/pull/44907#discussion_r4173335281
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx:
##########
@@ -398,6 +404,9 @@ export default function TableChart<D extends DataRecord =
DataRecord>(
const handleCellClicked = useCallback(
(event: CellClickedEvent) => {
+ const clickTarget = event.event?.target ?? null;
+ if (isJsonCellActionTarget(clickTarget)) return;
+ if (isJsonCellDoubleClick(event.event, clickTarget)) return;
Review Comment:
Everywhere else a double-click applies the filter and the second click
clears it again, but on a JSON cell the second click is dropped, so the filter
stays on. It also swallows a quick click-to-apply, click-to-clear on the same
cell, since that second click comes in with `detail === 2`.
Is this left over from when double-click opened the dialog? If so I'd drop
it along with `JSON_CELL_SELECTOR`.
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/renderers/TextCellRenderer.tsx:
##########
@@ -70,6 +78,26 @@ export const TextCellRenderer = (params: CellRendererProps)
=> {
}
}
+ const parsedJson = parseJsonCellValue(value);
+ if (parsedJson) {
Review Comment:
Text columns wrap by default (`truncateLongCells` off gives `wrapText` +
`autoHeight` in useColDefs), so on existing charts a JSON cell used to show the
whole value across lines. Now it's clipped to one line with an ellipsis,
there's no setting that brings the old view back, and report screenshots lose
the content too.
Could the preview wrap when the column has `wrapText`, or keep the old plain
rendering unless `jsonInCell` is on?
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/renderers/parseJsonCellValue.ts:
##########
@@ -0,0 +1,207 @@
+/**
+ * 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.
+ */
+
+export type JsonContainer = Record<string, unknown> | unknown[];
+
+// Visible cells are parsed on the render path. Above this size the cell stays
+// plain text. Parsed results are reused for the same string.
+const MAX_JSON_CELL_LENGTH = 100_000;
+const PARSE_CACHE_LIMIT = 200;
Review Comment:
Nit, take it or leave it. The limit counts entries, not size, so with values
near the 100 KB cap this module-level cache can hold around 20 MB of text plus
the parsed trees for the whole session.
Capping on total length instead of count would keep that bounded.
--
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]
