-
Notifications
You must be signed in to change notification settings - Fork 22
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.
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
base: master
Are you sure you want to change the base?
Changes from 3 commits
263c87f
592463f
00ccb52
a2f065b
5e63e6b
b5821ef
e57dbb6
b5e21c5
c197544
29943be
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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'; | ||||||
|
|
@@ -274,11 +275,21 @@ interface BasicRowContentProps extends CommonProps { | |||||
| atomicReading?: boolean; | ||||||
| } | ||||||
|
|
||||||
| type SwitchDisclosure = { | ||||||
| expanded: boolean; // is the related content expanded | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you want comments to be shown by vscode as jsdoc, you need to write them in jsdoc format: {
/** some doc about foo */
foo: string;
} |
||||||
| controlsId?: string; // id of the related content (if any) | ||||||
| live?: 'off' | 'polite' | 'assertive'; // announcement channel when changing expanded | ||||||
| busy?: boolean; // blocks UI and sets aria-busy | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer using |
||||||
| showSpinner?: boolean; // shows spinner on the right | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. one was for showing the spinner the other for setting aria-busy, but as the spinner is not in the spec, i'll delete the showSpinner |
||||||
| onLabelWhenExpanded?: string; // message when expanded = true | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we need a different label for the expanded state? can't this be controlled by Row users by changing the label when needed?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is for changing the announcement of the SR, in case you wanted something diffferent form "Options below" |
||||||
| }; | ||||||
|
|
||||||
| interface SwitchRowContentProps extends CommonProps { | ||||||
| onPress?: (() => void) | undefined; | ||||||
| trackingEvent?: TrackingEvent | ReadonlyArray<TrackingEvent>; | ||||||
|
|
||||||
| switch: ControlProps | undefined; | ||||||
| switchDisclosure?: SwitchDisclosure; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why a different |
||||||
| } | ||||||
|
|
||||||
| interface CheckboxRowContentProps extends CommonProps { | ||||||
|
|
@@ -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.'}` | ||||||
|
||||||
| : 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={props.switchDisclosure?.live !== 'off'} | ||||||
| aria-busy={rowIsBusy || undefined} | ||||||
|
||||||
| aria-busy={rowIsBusy || undefined} | |
| aria-busy={rowIsBusy ? 'true' : undefined} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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

There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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