sadpandajoe commented on code in PR #44668:
URL: https://github.com/apache/superset/pull/44668#discussion_r4134511971
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/types.ts:
##########
@@ -48,7 +48,15 @@ export interface ColumnConfigInfo {
export type ControlFormItemDefaultSpec = ControlFormItemSpec<
keyof typeof ControlFormItemComponents
->;
+> & {
+ /**
+ * Marks a checkbox whose value mirrors a chart-level option: when the
+ * per-column value is an explicit override, the popover offers a reset
+ * action that deletes the key from column_config so the column follows
+ * the chart-level setting again.
+ */
+ resettable?: boolean;
Review Comment:
`resettable` is typed generically on `ControlFormItemDefaultSpec`, so any
control spec can set `resettable: true`, but only the `Checkbox` branch in
`ControlFormItem` ever reads it. A resettable non-checkbox field would compile
cleanly and silently render no reset action. Would it be worth scoping the type
to checkbox specs, or wiring the reset UI for other control types too?
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ControlForm/ControlForm.reset.test.tsx:
##########
@@ -0,0 +1,115 @@
+/**
+ * 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 { render, screen, userEvent } from 'spec/helpers/testing-library';
+import ControlForm, {
+ ControlFormItem,
+ ControlFormRow,
+} from 'src/explore/components/controls/ColumnConfigControl/ControlForm';
+
+const CHECKBOX = (
+ <ControlFormItem
+ name="showCellBars"
+ controlType="Checkbox"
+ label="Show cell bars"
+ description="Whether to display a bar chart background in table columns"
+ defaultValue
+ debounceDelay={0}
Review Comment:
Every test in this file passes `debounceDelay={0}`, which routes through
`ControlForm`'s synchronous `0` bucket and never exercises `lodash.debounce`.
The three resettable checkboxes use a real 200ms delay in production, so this
suite can't catch a regression where a pending debounced write fires after a
reset and re-adds the key it just removed. Could you add a case using the
production 200ms delay with fake timers — toggle, reset within the window,
advance the timer, and assert the key is still gone?
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ControlForm/ControlFormItem.tsx:
##########
@@ -94,15 +113,35 @@ export function ControlFormItem({
onMouseLeave={() => setHovered(false)}
>
{controlType === 'Checkbox' ? (
- <ControlFormItemComponents.Checkbox
- value={value as boolean}
- onChange={handleChange}
- name={name}
- label={label}
- description={description}
- validationErrors={validationErrors}
- {...props}
- />
+ <>
+ <ControlFormItemComponents.Checkbox
+ value={value as boolean}
+ onChange={handleChange}
+ name={name}
+ label={label}
+ description={description}
+ validationErrors={validationErrors}
+ {...props}
+ />
+ {hasOverride && onReset && (
Review Comment:
Clicking reset removes the per-column override, which flips `hasOverride` to
false and unmounts this button on the next render — a keyboard user who just
activated it loses focus to the document instead of staying inside the popover.
Could focus move to the checkbox (or another stable element) right after reset
instead of being dropped?
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ControlForm/ControlFormItem.tsx:
##########
@@ -94,15 +113,35 @@ export function ControlFormItem({
onMouseLeave={() => setHovered(false)}
>
{controlType === 'Checkbox' ? (
- <ControlFormItemComponents.Checkbox
- value={value as boolean}
- onChange={handleChange}
- name={name}
- label={label}
- description={description}
- validationErrors={validationErrors}
- {...props}
- />
+ <>
+ <ControlFormItemComponents.Checkbox
+ value={value as boolean}
+ onChange={handleChange}
+ name={name}
+ label={label}
+ description={description}
+ validationErrors={validationErrors}
+ {...props}
+ />
+ {hasOverride && onReset && (
+ <div
+ css={{
+ marginTop: sizeUnit,
+ paddingLeft: sizeUnit * 6,
+ }}
+ >
+ <Button
+ type="link"
+ size="small"
+ css={{ padding: 0, height: 'auto', fontSize: fontSizeXS }}
+ onClick={onReset}
+ >
+ <Icons.RollbackOutlined iconSize="s" />{' '}
Review Comment:
`Icons.RollbackOutlined` carries an implicit, untranslated label, so a
screen reader may announce something like "rollback Use the chart-level
setting" instead of just the translated action text. Could the icon be marked
decorative (e.g. `aria-hidden`) so the button's text alone supplies its
accessible name?
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ControlForm/index.tsx:
##########
@@ -115,6 +123,16 @@ export default function ControlForm({
[name]: fieldValue,
});
},
+ ...(onReset
+ ? {
+ onReset() {
+ if (onItemReset) {
+ onItemReset();
+ }
+ onReset(name);
+ },
+ }
+ : {}),
Review Comment:
Agreed—confirmed by reproducing it: toggle a resettable checkbox (starting
the ~200ms debounce), then click "Use the chart-level setting" before it fires.
The reset removes the key immediately, but the already-scheduled write still
lands afterward with the stale pre-reset value and silently re-adds the key.
Should the reset path cancel or flush any pending debounced write for that
field first, or route resets through the same debounced channel as toggles?
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ControlForm/ControlFormItem.tsx:
##########
@@ -47,16 +51,25 @@ export function ControlFormItem({
width,
validators,
onChange,
+ onReset,
value: initialValue,
defaultValue,
controlType,
+ resettable = false,
...props
}: ControlFormItemProps) {
- const { sizeUnit } = useTheme();
+ const { sizeUnit, fontSizeXS } = useTheme();
const [hovered, setHovered] = useState(false);
const [value, setValue] = useState(
initialValue === undefined ? defaultValue : initialValue,
);
+ const [prevInitialValue, setPrevInitialValue] = useState(initialValue);
+ if (initialValue !== prevInitialValue) {
+ // the parent value changed outside this item (e.g. the reset action
+ // removed the key) — follow it instead of keeping the stale local state
+ setPrevInitialValue(initialValue);
+ setValue(initialValue === undefined ? defaultValue : initialValue);
+ }
Review Comment:
Agreed—after a reset, the checkbox's local value falls back to the item
spec's static `defaultValue` (e.g. `true` for showCellBars), not the actual
current chart-level setting, since that value is never passed into this
component. If the chart-level option differs from the spec default, the
checkbox can show the wrong effective state right after reset. Could the
popover pass the resolved chart-level value down as the fallback instead of the
static per-field default?
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ColumnConfigPopover.tsx:
##########
@@ -79,7 +79,17 @@ export default function ColumnConfigPopover({
key: i.toString(),
label: item.tab,
children: (
- <ControlForm onChange={onChange} value={column.config}>
+ <ControlForm
+ onChange={onChange}
+ onReset={name =>
+ onChange(
+ Object.fromEntries(
+ Object.entries(column.config).filter(([key]) => key !==
name),
+ ),
+ )
+ }
Review Comment:
Agreed—both `ControlForm` call sites in this file implement the exact same
key-deletion closure. Since `ControlForm` already owns the value and calls
`onChange` with the full object on every change, would it make more sense for
the `onReset` channel to delete the key internally and call `onChange` itself,
rather than requiring every caller to reimplement the same filter?
--
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]
