EnxDev commented on code in PR #44907:
URL: https://github.com/apache/superset/pull/44907#discussion_r4189012451
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/consts.ts:
##########
@@ -45,6 +45,13 @@ export const FILTER_CONDITION_BODY_INDEX = {
export const ROW_NUMBER_COL_ID = '__row_number__';
+// Expand and view controls inside a JSON cell. A click on them skips
+// cross-filtering and row selection.
+export const JSON_CELL_ACTION_SELECTOR = '[data-json-cell-action]';
+
+// The control that opens the JSON dialog. Enter on the focused cell clicks it.
+export const JSON_CELL_OPEN_SELECTOR = '[data-test="json-cell-open"]';
Review Comment:
The plugin's esm build (`scripts/build.js`) runs babel with
`NODE_ENV=production`, and that env strips `data-test` through
`babel-plugin-jsx-remove-data-test-id`. So in the published package this
selector matches nothing and Enter does nothing. The app bundle goes through
SWC and keeps the attribute, which is why it works locally.
Could the open button get its own marker, the way `data-json-cell-action`
does? `JsonActionButton` would set it, and the two tests would use it instead
of `data-test`.
```suggestion
export const JSON_CELL_OPEN_SELECTOR = '[data-json-cell-open]';
```
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/isJsonCellActionTarget.ts:
##########
@@ -0,0 +1,55 @@
+/**
+ * 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 { JSON_CELL_ACTION_SELECTOR, JSON_CELL_OPEN_SELECTOR } from '../consts';
+
+export function isJsonCellActionTarget(target: EventTarget | null): boolean {
+ return (
+ target instanceof Element &&
+ target.closest(JSON_CELL_ACTION_SELECTOR) !== null
+ );
+}
+
+/** Enter on a focused JSON cell opens the dialog. The grid keeps Tab on
cells. */
+export function openJsonDialogOnEnter(
+ nativeEvent: Event | null | undefined,
+): boolean {
+ if (
+ !nativeEvent ||
+ !('key' in nativeEvent) ||
+ nativeEvent.key !== 'Enter' ||
+ ('ctrlKey' in nativeEvent && nativeEvent.ctrlKey) ||
+ ('metaKey' in nativeEvent && nativeEvent.metaKey) ||
+ ('altKey' in nativeEvent && nativeEvent.altKey)
+ ) {
+ return false;
+ }
+ const { target } = nativeEvent;
+ if (!(target instanceof Element)) {
+ return false;
Review Comment:
A mouse click on the arrow leaves focus on the button, and AG Grid walks up
from the target, so that keydown still reaches `onCellKeyDown`. Enter on a
focused arrow, nested ones in the tree included, gets `preventDefault`, the
arrow never toggles, and the dialog opens instead.
Skipping targets that are already one of the JSON controls lets the browser
activate whichever button has focus:
```suggestion
if (!(target instanceof Element) || isJsonCellActionTarget(target)) {
return false;
}
```
--
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]
