FrancescoCastaldi commented on code in PR #44945:
URL: https://github.com/apache/superset/pull/44945#discussion_r4258811290
##########
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:
Typed the dynamic component reference as ComponentType<any> and removed the
@ts-expect-error directive. This resolves both TS2578 and TS2322 cleanly across
tsc without leaving unsuppressed prop errors.
##########
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:
Updated to forward initialValue conditionally via {...(controlType ===
'RadioButtonControl' ? { initialValue } : {})}. Other dynamic controls such as
Input, InputNumber, and Select no longer receive initialValue, preventing
unknown-prop DOM attribute warnings.
##########
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:
Added ControlForm.test.tsx verifying this exact flow: mounting ControlForm
with horizontalAlign unset and defaultValue: 'right', clicking the displayed
Right option, asserting the single committed { horizontalAlign: 'right' } event
after debounce, re-rendering with the committed value, and asserting that a
subsequent 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]
