Skip to content

feat(Row): Row switch announces when it has expanded content #1462

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
AlexandraGallipoliRodrigues wants to merge 10 commits into
base: master
Choose a base branch
from
Open

feat(Row): Row switch announces when it has expanded content #1462

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
263c87f
row-switch-announces-when-it-has-expanded-content
AlexandraGallipoliRodrigues Nov 18, 2025
592463f
translated comments
AlexandraGallipoliRodrigues Nov 18, 2025
00ccb52
modified aria-atomic
AlexandraGallipoliRodrigues Nov 18, 2025
a2f065b
WEB-2283: replaced literal with a token
AlexandraGallipoliRodrigues Nov 19, 2025
5e63e6b
Merge branch 'master' into row-switch-announces-when-it-has-expanded-…
AlexandraGallipoliRodrigues Nov 21, 2025
b5821ef
PR changes
AlexandraGallipoliRodrigues Dec 9, 2025
e57dbb6
Merge branch 'master' of github.com:Telefonica/mistica-web into row-s…
AlexandraGallipoliRodrigues Dec 9, 2025
b5e21c5
minor changes
AlexandraGallipoliRodrigues Dec 10, 2025
c197544
added private list-disclosure-story
AlexandraGallipoliRodrigues Dec 10, 2025
29943be
deleted aria-expanded and aria-controls from rows
AlexandraGallipoliRodrigues Dec 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 52 additions & 17 deletions src/list.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {applyCssVars} from './utils/css';
import {IconButton, ToggleIconButton} from './icon-button';
import ScreenReaderOnly from './screen-reader-only';
import {useTheme} from './hooks';
import Spinner from './spinner';

import type {IconButtonProps, ToggleIconButtonProps} from './icon-button';
import type {TouchableElement, TouchableProps} from './touchable';
Expand Down Expand Up @@ -274,11 +275,21 @@ interface BasicRowContentProps extends CommonProps {
atomicReading?: boolean;
}

type SwitchDisclosure = {

@aweell aweell Nov 27, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider a more open approach rather than the row with switch only?

I was reviewing requests from design teams and in this case the control used is a radio instead of a switch:

Telefonica/mistica-design#2172

Can (or should) this approach be abstracted to work with any type of control the list allows?

@atabel atabel Nov 27, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree, I think we should make this more generic

expanded: boolean; // ¿hay contenido mostrado debajo?
controlsId?: string; // id de la región expandida (si existe)
live?: 'off' | 'polite' | 'assertive'; // canal de anuncio al cambiar expanded
busy?: boolean; // bloquea UI y marca aria-busy
showSpinner?: boolean; // muestra spinner en la derecha
onLabelWhenExpanded?: string; // mensaje cuando expanded = true
};

interface SwitchRowContentProps extends CommonProps {
onPress?: (() => void) | undefined;
trackingEvent?: TrackingEvent | ReadonlyArray<TrackingEvent>;

switch: ControlProps | undefined;
switchDisclosure?: SwitchDisclosure;

@atabel atabel Nov 27, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why a different switchDisclosure prop? shouldn't most of that props be part of the ControlProps type?

}

interface CheckboxRowContentProps extends CommonProps {
Expand Down Expand Up @@ -545,38 +556,62 @@ const RowContent = React.forwardRef<TouchableElement, RowContentProps>((props, r
if (props.switch || props.checkbox) {
const Control = props.switch ? Switch : Checkbox;
const name = props.switch?.name ?? props.checkbox?.name ?? titleId;
const computedAriaLabel =
ariaLabel ??
(props.switchDisclosure?.expanded
? `${title} ${props.switchDisclosure?.onLabelWhenExpanded ?? 'Options available below.'}`

Copilot AI Nov 18, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fallback text 'Options available below.' is hardcoded in English. This should be internationalized or derived from a translation function to support multiple languages, as the PR description shows usage with I18N.translate() in the webapp integration examples.

Copilot uses AI. Check for mistakes.

@AlexandraGallipoliRodrigues AlexandraGallipoliRodrigues Nov 18, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i haven't found any I18N.translate() in this repo. How should I approach this?

@pladaria pladaria Nov 19, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Search in the repo for usage examples of: const {texts, t} = useTheme();

Texts are defined in the context provider and can be overriden, for example:

texts.formCreditCardNumberLabel || t(tokens.formCreditCardNumberLabel)
  • texts.formCreditCardNumberLabel is the user defined text override via provider (optional)
  • t(tokens.formCreditCardNumberLabel) calls the translate function for the token

Comment thread
pladaria marked this conversation as resolved.
Outdated
: ariaLabel);
const rowIsBusy = !!props.switchDisclosure?.busy;
const showSpinner = !!props.switchDisclosure?.showSpinner;

return isInteractive
? renderRowWithDoubleInteraction(
<Control
disabled={disabled}
disabled={disabled || rowIsBusy}
name={name}
checked={isChecked}
aria-label={ariaLabel}
aria-label={computedAriaLabel}
aria-labelledby={titleId}
aria-controls={props.switchDisclosure?.controlsId}
aria-expanded={props.switchDisclosure?.expanded}
onChange={toggle}
render={({controlElement}) => (
<div className={styles.dualActionRight}>{controlElement}</div>
)}
/>
)
: renderRowWithSingleControl(
<Control
disabled={disabled}
name={name}
checked={isChecked}
aria-label={ariaLabel}
aria-labelledby={titleId}
onChange={toggle}
render={({controlElement, labelId}) => (
<Box paddingX={16} role={role}>
{renderContent({
labelId,
control: <Stack space="around">{controlElement}</Stack>,
})}
</Box>
<div
aria-live={props.switchDisclosure?.live ?? 'off'}
aria-atomic

Copilot AI Nov 18, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The aria-atomic attribute is always set to true, but aria-live defaults to 'off'. When aria-live is 'off', aria-atomic has no effect. Consider only setting aria-atomic when aria-live is not 'off', or documenting why aria-atomic should always be present.

Suggested change
aria-atomic
{...(props.switchDisclosure?.live && props.switchDisclosure.live !== 'off' ? {'aria-atomic': true} : {})}

Copilot uses AI. Check for mistakes.

@AlexandraGallipoliRodrigues AlexandraGallipoliRodrigues Nov 18, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have adjusted aria-atomic so that it is only added when aria-live is not “off”.
In practice, our case always uses live: “assertive”, but we leave the type open to “polite” | “off” | “assertive”, so the wrapper behaves correctly in any of them

aria-busy={rowIsBusy || undefined}

Copilot AI Nov 18, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using || undefined to conditionally render the attribute is unnecessary. The aria-busy attribute should be explicitly set to 'true' or 'false' as a string, or omitted entirely. Use aria-busy={rowIsBusy ? 'true' : undefined} or aria-busy={rowIsBusy ? true : undefined} instead.

Suggested change
aria-busy={rowIsBusy || undefined}
aria-busy={rowIsBusy ? 'true' : undefined}

Copilot uses AI. Check for mistakes.

@AlexandraGallipoliRodrigues AlexandraGallipoliRodrigues Nov 18, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i thought is a type Booleanish = boolean | 'true' | 'false'; so booleans are also valid, right?

@pladaria pladaria Nov 26, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so

Comment on lines +613 to +616

@atabel atabel Nov 27, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this aria-live/atomic/busy part not needed in the renderRowWithDoubleInteraction case?

@AlexandraGallipoliRodrigues AlexandraGallipoliRodrigues Dec 9, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's needed as well, i'll added it

>
<Control
disabled={disabled || rowIsBusy}
name={name}
checked={isChecked}
aria-label={computedAriaLabel}
aria-labelledby={titleId}
aria-controls={props.switchDisclosure?.controlsId}
aria-expanded={props.switchDisclosure?.expanded}

@atabel atabel Nov 27, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't work. You need to add these aria props in Switch component. TS doesn't warn you because there is a TS bug that allows you passing any aria-xxx prop to any component

@AlexandraGallipoliRodrigues AlexandraGallipoliRodrigues Dec 9, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oki, but for the role row it seems that aria doesn't support aria-expanded
image

i'll added inside the div before the RadioButton

onChange={toggle}
render={({controlElement, labelId}) => (
<Box paddingX={16} role={role}>
{renderContent({
labelId,
control: <Stack space="around">{controlElement}</Stack>,
})}
</Box>
)}
/>
{showSpinner && (

@atabel atabel Nov 27, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this spinner in the spec?

@aweell aweell Dec 2, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recall the spinner was part of other conversation rather than expand/collapse states.

Telefonica/mistica-design#2151

The row is also using a switch but not necessarily expands or collapses content. If we want to cover coth cases i can update specs.

https://teams.microsoft.com/l/message/19:68854aea147e4787803aff0e380e1208@thread.tacv2/1739980344078?tenantId=9744600e-3e04-492e-baa1-25ec245c6f10&groupId=e265fe99-929f-45d1-8154-699649674a40&parentMessageId=1739980344078&teamName=M%C3%ADstica&channelName=Help&createdTime=1739980344078

@AlexandraGallipoliRodrigues AlexandraGallipoliRodrigues Dec 9, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you’re right, the spinner is not part of the task, it comes from the original call-forwarding loading states (CHANGING / LENGTHY), which is a separate matter.

I’ll remove showSpinner and the built–in spinner rendering from Row

<div className={styles.rightContent}>
<div className={styles.center}>
<Spinner aria-hidden />
</div>
</div>
)}
/>,
</div>,
true
);
}
Expand Down
Loading