sadpandajoe commented on code in PR #44945:
URL: https://github.com/apache/superset/pull/44945#discussion_r4257524806
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ControlForm/ControlFormItem.tsx:
##########
@@ -115,7 +115,12 @@ export function ControlFormItem({
/>
)}
{/* @ts-expect-error - dynamic Control component has varying prop
types */}
- <Control {...props} value={value} onChange={handleChange} />
+ <Control
+ {...props}
+ value={value}
+ initialValue={initialValue}
Review Comment:
`initialValue` is now passed to every dynamic control, not just the radio
one. `Input`, `InputNumber` and `Select` forward unknown props to their
underlying DOM elements, so opening a column popover with a saved display name
or min width will log React's unknown-prop warning and put a stray
`initialvalue` attribute on the element. Would it make sense to pass this only
when `controlType === 'RadioButtonControl'`?
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ControlForm/ControlFormItem.tsx:
##########
@@ -115,7 +115,12 @@ export function ControlFormItem({
/>
)}
{/* @ts-expect-error - dynamic Control component has varying prop
types */}
- <Control {...props} value={value} onChange={handleChange} />
+ <Control
+ {...props}
+ value={value}
Review Comment:
Splitting this JSX element across lines leaves the `@ts-expect-error` above
only covering the `<Control` line, so the type errors on `value` and `onChange`
now land on unsuppressed lines. The `lint-frontend` `tsc` step is currently
failing on exactly this (TS2578 unused directive at 117, TS2322 at 120 and
122). Can the suppression be moved so it covers the prop errors (or the props
typed so no suppression is needed)?
##########
superset-frontend/packages/superset-ui-chart-controls/test/shared-controls/components/RadioButtonControl.test.tsx:
##########
@@ -95,22 +95,80 @@ test('calls onChange when radio button is clicked', () => {
const secondOption = screen.getByText('Option 2');
fireEvent.click(secondOption);
+ expect(onChange).toHaveBeenCalledTimes(1);
expect(onChange).toHaveBeenCalledWith('option2');
- expect(onChange).toHaveBeenCalled();
+});
+
+test('does not call onChange when clicking the already selected option', () =>
{
+ const onChange = jest.fn();
+ setup({ value: 'option1', onChange });
+
+ const firstOption = screen.getByText('Option 1');
+ fireEvent.click(firstOption);
+
+ expect(onChange).not.toHaveBeenCalled();
+});
+
+test('calls onChange when clicking the displayed default option if value is
unpersisted', () => {
+ const onChange = jest.fn();
+ setup({ value: undefined, onChange });
+
+ const firstOption = screen.getByText('Option 1');
+ fireEvent.click(firstOption);
+
+ expect(onChange).toHaveBeenCalledTimes(1);
+ expect(onChange).toHaveBeenCalledWith('option1');
+});
+
+test('calls onChange when clicking the displayed default option when
initialValue is unpersisted even if value is seeded with default', () => {
Review Comment:
These tests hand `initialValue` straight to `RadioButtonControl`, so they
don't cover the path that actually motivated the fix: `ControlFormItem` seeding
`defaultValue` into local state and the parent round-tripping the committed
value. If the new `initialValue={initialValue}` forwarding in `ControlFormItem`
were removed, every test here would still pass. Could you add a `ControlForm`
test with `horizontalAlign` unset and `defaultValue: 'right'` that clicks the
highlighted Right option, asserts one `{ horizontalAlign: 'right' }` commit
after the debounce, then re-renders with that value and asserts a second click
commits nothing?
--
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]
