sadpandajoe commented on code in PR #44945:
URL: https://github.com/apache/superset/pull/44945#discussion_r4342839615
##########
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/components/RadioButtonControl.tsx:
##########
@@ -100,7 +105,9 @@ export default function RadioButtonControl({
}`}
onClick={e => {
e.currentTarget?.focus();
- onChange(val);
+ if (!disabled && val === currentValue) {
+ onChange(val);
Review Comment:
Clicking the option that is already selected still calls `onChange` here,
even when that value is already persisted. For example, with `map_view.mode =
'FIT_DATA'` already saved, clicking **Fit data** dispatches an equivalent
update and re-renders the control, which is the second symptom described in
#44921 and the opposite of the "not called when clicking an already-selected
option" behavior in the PR description. The tests at
`RadioButtonControl.test.tsx:102` and `ControlForm.test.tsx` now require this
callback. Is that intended? If only an unpersisted displayed default needs
committing, could the condition be limited to that case, or could the
description and tests be updated to state the real contract?
##########
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/components/RadioButtonControl.tsx:
##########
@@ -58,22 +59,26 @@ function normalizeOption(option: RadioButtonOption):
RadioButtonOptionObject {
return option;
}
-export default function RadioButtonControl({
- value: initialValue,
- options,
- onChange,
- ...props
-}: RadioButtonControlProps) {
+export default function RadioButtonControl(props: RadioButtonControlProps) {
+ const {
+ value,
+ options,
+ onChange,
+ initialValue: _initialValue,
Review Comment:
`initialValue` is added to `RadioButtonControlProps` and then discarded
here, so a caller that passes it gets no effect (for example
`initialValue="option2"` with no `value` still shows the first option). The new
`initialValue` test cases only repeat the selected-click case and do not
exercise any persistence-dependent behavior. Is this prop needed, or should it
be removed until it has defined behavior?
--
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]
