mikebridge commented on code in PR #44452:
URL: https://github.com/apache/superset/pull/44452#discussion_r4209084340
##########
superset-frontend/src/explore/components/controls/CheckboxControl.tsx:
##########
@@ -50,14 +53,30 @@ const CheckBoxControlWrapper = styled.div`
export default function CheckboxControl({
value = false,
label,
+ disabled = false,
+ disabledReason,
+ resetLabel,
onChange = () => {},
...restProps
}: CheckboxControlProps): JSX.Element {
+ const explanationId = useId();
const handleChange = useCallback((): void => {
- onChange(!value);
- }, [onChange, value]);
+ if (!disabled) {
+ onChange(!value);
+ }
+ }, [disabled, onChange, value]);
- const checkbox = <Checkbox onChange={handleChange} checked={!!value} />;
+ const checkbox = (
+ <Checkbox
+ onChange={handleChange}
+ checked={!!value}
+ disabled={disabled}
+ aria-describedby={disabled && disabledReason ? explanationId : undefined}
+ />
Review Comment:
Good catch. `aria-describedby` explained the disabled state but did not name
the checkbox, and the outer label had no matching input ID.
Could we link the existing visible label directly? Commit 4ff18ddf gives the
input an ID and passes it to `ControlHeader`; the accessible-name and
description tests pass for both string and ReactNode labels.
##########
superset-frontend/src/explore/components/controls/CheckboxControl.test.tsx:
##########
@@ -49,4 +49,37 @@ describe('CheckboxControl', () => {
await userEvent.click(label);
expect(defaultProps.onChange).toHaveBeenCalled();
});
+
+ test('explains a disabled checkbox without a label', () => {
+ render(
+ setup({
+ label: undefined,
+ disabled: true,
+ disabledReason: 'Unavailable',
+ }),
+ );
+
+ const checkbox = screen.getByRole('checkbox');
+ expect(checkbox).toHaveAccessibleDescription('Unavailable');
+ expect(screen.getByText('Unavailable')).toBeInTheDocument();
+ });
+
+ test('retains the control header and description when disabled', () => {
+ const { container } = render(
+ setup({
+ disabled: true,
+ description: 'Why this control is disabled',
+ hovered: true,
+ renderTrigger: true,
+ }),
+ );
+
+ expect(
+ container.querySelector('[data-test="show_legend-header"]'),
+ ).toBeInTheDocument();
+ expect(
+ container.querySelector('[data-test="show_legend-description-icon"]'),
+ ).toBeInTheDocument();
+ expect(screen.getByRole('checkbox')).toBeDisabled();
+ });
Review Comment:
Good catch on the missing regression pin. Could we keep the defensive
`handleChange` guard as well as the disabled label click restriction?
Commit 4ff18ddf adds a disabled-label test that fails when both guards are
removed, and the enabled-label test now asserts exactly one callback so the
label association does not double-toggle.
##########
superset-frontend/src/explore/components/controls/CheckboxControl.tsx:
##########
@@ -50,24 +53,58 @@ const CheckBoxControlWrapper = styled.div`
export default function CheckboxControl({
value = false,
label,
+ disabled = false,
+ disabledReason,
+ resetLabel,
onChange = () => {},
...restProps
}: CheckboxControlProps): JSX.Element {
+ const explanationId = useId();
+ const generatedCheckboxId = useId();
+ const checkboxId = restProps.name || generatedCheckboxId;
const handleChange = useCallback((): void => {
- onChange(!value);
- }, [onChange, value]);
+ if (!disabled) {
+ onChange(!value);
+ }
+ }, [disabled, onChange, value]);
- const checkbox = <Checkbox onChange={handleChange} checked={!!value} />;
+ const checkbox = (
+ <Checkbox
+ id={checkboxId}
+ onChange={handleChange}
+ checked={!!value}
+ disabled={disabled}
+ aria-describedby={disabled && disabledReason ? explanationId : undefined}
+ />
+ );
+ const explanation = disabled && disabledReason && (
+ <p id={explanationId}>{disabledReason}</p>
+ );
if (label) {
return (
<CheckBoxControlWrapper>
<ControlHeader
{...restProps}
+ name={checkboxId}
label={label}
leftNode={checkbox}
- onClick={handleChange}
+ onClick={disabled ? undefined : handleChange}
Review Comment:
Good catch. Could you take another look at the [follow-up
commit](https://github.com/apache/superset/commit/7fabdec758505f130a814f79c5979dd83f43a383)?
The label-text click prevents the second native activation and explicitly
focuses the checkbox, preserving the label behavior. The new click and focus
regressions failed against the broken variants and pass with the fix; the
new-head CI is running.
--
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]
