Skip to content
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

feat(StructuredList): Accessibility refactor #8172

Merged
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
6954ba0
fix(StructuredList): refactor instanceId
dakahn Mar 18, 2021
cb09d20
fix(StructuredList): convert to radio interaction, add focus state
dakahn Mar 23, 2021
1f46ed5
fix(StructuredList): hide radio input
dakahn Mar 23, 2021
f655b31
fix(StructuredList): remove label and tabIndex props on input
dakahn Mar 23, 2021
156cbc1
test(StructuredList): update tests, remove label tests
dakahn Mar 23, 2021
bd86cea
fix(StructuredList): style changes, add deprecation warning
dakahn Mar 24, 2021
909726a
fix(StructuredList): add context for row click handler
dakahn Mar 25, 2021
fb68deb
test(StructuredList): add specificity to classname testing
dakahn Mar 30, 2021
48ef999
fix(StructuredList): add clearer deprecation warning for defaultChecked
dakahn Mar 30, 2021
15310ad
Merge branch 'main' into 1937-improved-selection-a11y
dakahn Mar 30, 2021
fd90e92
fix(structuredList): updated style
andreancardona Mar 31, 2021
5af34a2
fix(structuredList): updated style again
andreancardona Mar 31, 2021
0487407
Merge branch 'main' of github.com:carbon-design-system/carbon into 19…
dakahn Apr 1, 2021
606c728
Update packages/react/src/components/StructuredList/StructuredList.js
dakahn Apr 1, 2021
c2184ea
Merge branch 'main' into 1937-improved-selection-a11y
dakahn Apr 1, 2021
2673889
fix(StructuredList): update render return to be in carbon style
dakahn Apr 1, 2021
7d835f4
Merge branch '1937-improved-selection-a11y' of github.com:dakahn/carb…
dakahn Apr 1, 2021
c151da7
Merge branch 'main' of github.com:carbon-design-system/carbon into 19…
dakahn Apr 6, 2021
84fa755
Merge branch 'main' of github.com:carbon-design-system/carbon into 19…
dakahn Apr 7, 2021
2cdf113
fix(StructuredList): move new work to /next
dakahn Apr 7, 2021
08ce487
fix(StructuredList): put changes behind feature flag
dakahn Apr 7, 2021
110e400
Merge branch 'main' of github.com:carbon-design-system/carbon into 19…
dakahn Apr 9, 2021
e6a9615
test(StructuredList): add old version tests
dakahn Apr 9, 2021
07eda0d
fix(StructuredList): update to Carbon naming convention
dakahn Apr 12, 2021
7770b66
test(StructuredList): update snapshots
dakahn Apr 12, 2021
12c3862
test(StructuredList): address merge conflicts
dakahn Apr 12, 2021
939bcfd
Merge branch 'main' of github.com:carbon-design-system/carbon into 19…
dakahn Apr 12, 2021
4d9c07c
Merge branch 'main' into 1937-improved-selection-a11y
dakahn Apr 13, 2021
182a935
Merge branch 'main' into 1937-improved-selection-a11y
andreancardona Apr 14, 2021
1789d5d
Merge branch 'main' into 1937-improved-selection-a11y
dakahn Apr 14, 2021
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
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@
@include padding--data-structured-list;
}

.#{$prefix}--structured-list-input {
display: none;
.#{$prefix}--structured-list-row--focused-within {
@include focus-outline('outline');
}

.#{$prefix}--structured-list {
Expand Down Expand Up @@ -62,10 +62,6 @@
cursor: inherit;
}

.#{$prefix}--structured-list-row:focus:not(.#{$prefix}--structured-list-row--header-row) {
@include focus-outline('outline');
}

.#{$prefix}--structured-list--selection
.#{$prefix}--structured-list-row:hover:not(.#{$prefix}--structured-list-row--header-row)
> .#{$prefix}--structured-list-td,
Expand Down Expand Up @@ -113,6 +109,7 @@
display: table-cell;
max-width: 36rem;
color: $text-02;
border-top: 1px solid $ui-03;
transition: color $duration--fast-02 motion(standard, productive);
}

Expand Down
34 changes: 4 additions & 30 deletions packages/react/__tests__/__snapshots__/PublicAPI-test.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -5000,9 +5000,7 @@ Map {
"StructuredListRow" => Object {
"defaultProps": Object {
"head": false,
"label": false,
"onKeyDown": [Function],
"tabIndex": 0,
},
"propTypes": Object {
"children": Object {
Expand All @@ -5014,56 +5012,32 @@ Map {
"head": Object {
"type": "bool",
},
"label": Object {
"type": "bool",
},
"label": [Function],
"onKeyDown": Object {
"type": "func",
},
"tabIndex": Object {
"type": "number",
},
},
},
"StructuredListInput" => Object {
"defaultProps": Object {
"onChange": [Function],
"title": "title",
"value": "value",
},
"propTypes": Object {
"className": Object {
"type": "string",
},
"defaultChecked": Object {
"type": "bool",
},
"defaultChecked": [Function],
"id": Object {
"type": "string",
},
"name": Object {
"type": "string",
},
"onChange": Object {
"type": "func",
},
"onChange": [Function],
"title": Object {
"type": "string",
},
"value": Object {
"args": Array [
Array [
Object {
"type": "string",
},
Object {
"type": "number",
},
],
],
"isRequired": true,
"type": "oneOfType",
},
"value": [Function],
},
},
"StructuredListCell" => Object {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,13 @@ describe('StructuredListWrapper', () => {
);

it('should have the expected classes', () => {
expect(wrapper.hasClass(`${prefix}--structured-list`)).toEqual(true);
expect(
wrapper.find('div').hasClass(`${prefix}--structured-list`)
).toEqual(true);
});

it('Should add extra classes that are passed via className', () => {
expect(wrapper.hasClass('extra-class')).toEqual(true);
expect(wrapper.find('div').hasClass('extra-class')).toEqual(true);
});

it('By default, selection prop is false', () => {
Expand All @@ -41,9 +43,9 @@ describe('StructuredListWrapper', () => {

it('Should add the modifier class for selection when selection prop is true', () => {
wrapper.setProps({ selection: true });
expect(wrapper.hasClass(`${prefix}--structured-list--selection`)).toEqual(
true
);
expect(
wrapper.find('div').hasClass(`${prefix}--structured-list--selection`)
).toEqual(true);
});
});
});
Expand Down Expand Up @@ -120,16 +122,6 @@ describe('StructuredListRow', () => {
).toEqual(true);
});

it('should use <div> HTML by default (or when label prop is false)', () => {
const wrapperLabel = shallow(<StructuredListRow />);
expect(wrapperLabel.getElement().type).toEqual('div');
});

it('should use <label> HTML when label prop is true', () => {
const wrapperLabel = shallow(<StructuredListRow label />);
expect(wrapperLabel.getElement().type).toEqual('label');
});

it('Should accept other props from ...other', () => {
const wrapperProps = shallow(
<StructuredListRow title="title">hi</StructuredListRow>
Expand Down
129 changes: 83 additions & 46 deletions packages/react/src/components/StructuredList/StructuredList.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,16 @@
* LICENSE file in the root directory of this source tree.
*/

import React from 'react';
import React, { useState } from 'react';
import PropTypes from 'prop-types';
import classNames from 'classnames';
import { settings } from 'carbon-components';
import setupGetInstanceId from '../../tools/setupGetInstanceId';
import { useId } from '../../internal/useId';
import deprecate from '../../prop-types/deprecate';

const { prefix } = settings;
const getInstanceId = setupGetInstanceId();
const GridSelectedRowStateContext = React.createContext(null);
const GridSelectedRowDispatchContext = React.createContext(null);

export function StructuredListWrapper(props) {
const {
Expand All @@ -27,11 +28,16 @@ export function StructuredListWrapper(props) {
const classes = classNames(`${prefix}--structured-list`, className, {
[`${prefix}--structured-list--selection`]: selection,
});
const [selectedRow, setSelectedRow] = React.useState(null);

return (
<div role="table" className={classes} {...other} aria-label={ariaLabel}>
{children}
</div>
<GridSelectedRowStateContext.Provider value={selectedRow}>
<GridSelectedRowDispatchContext.Provider value={setSelectedRow}>
<div role="grid" className={classes} {...other} aria-label={ariaLabel}>
{children}
</div>
</GridSelectedRowDispatchContext.Provider>
</GridSelectedRowStateContext.Provider>
);
}

Expand Down Expand Up @@ -127,32 +133,40 @@ StructuredListBody.defaultProps = {
onKeyDown: () => {},
};

const GridRowContext = React.createContext(null);

export function StructuredListRow(props) {
const {
onKeyDown,
tabIndex,
children,
className,
head,
label,
...other
} = props;
const { onKeyDown, children, className, head, ...other } = props;
const [hasFocusWithin, setHasFocusWithin] = useState(false);
const id = useId('grid-input');
const setSelectedRow = React.useContext(GridSelectedRowDispatchContext);
const value = { id };
const classes = classNames(`${prefix}--structured-list-row`, className, {
[`${prefix}--structured-list-row--header-row`]: head,
[`${prefix}--structured-list-row--focused-within`]: hasFocusWithin,
});

return label ? (
// eslint-disable-next-line jsx-a11y/no-noninteractive-element-interactions
<label
return head ? (
<div role="row" {...other} className={classes}>
{children}
</div>
) : (
// eslint-disable-next-line jsx-a11y/interactive-supports-focus
<div
{...other}
tabIndex={tabIndex}
role="row"
className={classes}
onClick={() => setSelectedRow(id)}
onFocus={() => {
setHasFocusWithin(true);
}}
onBlur={() => {
setHasFocusWithin(false);
}}
onKeyDown={onKeyDown}>
{children}
</label>
) : (
<div {...other} className={classes}>
{children}
<GridRowContext.Provider value={value}>
{children}
</GridRowContext.Provider>
</div>
);
}
Expand All @@ -176,40 +190,52 @@ StructuredListRow.propTypes = {
/**
* Specify whether a `<label>` should be used
*/
label: PropTypes.bool,
dakahn marked this conversation as resolved.
Show resolved Hide resolved
label: deprecate(
PropTypes.bool,
`\nThe prop \`label\` will be removed in the next major version of Carbon.`
dakahn marked this conversation as resolved.
Show resolved Hide resolved
),

/**
* Provide a handler that is invoked on the key down event for the control,
* if `<label>` is in use
*/
onKeyDown: PropTypes.func,

/**
* Specify the tab index of the container node, if `<label>` is in use
*/
tabIndex: PropTypes.number,
};

StructuredListRow.defaultProps = {
head: false,
label: false,
tabIndex: 0,
onKeyDown: () => {},
};

export function StructuredListInput(props) {
const { className, value, name, title, id, ...other } = props;
const classes = classNames(`${prefix}--structured-list-input`, className);
const instanceId = id || getInstanceId();
const defaultId = useId('structureListInput');
const {
className,
name = `structured-list-input-${defaultId}`,
title,
id,
...other
} = props;
const classes = classNames(
`${prefix}--structured-list-input`,
`${prefix}--visually-hidden`,
className
);
const row = React.useContext(GridRowContext);
const selectedRow = React.useContext(GridSelectedRowStateContext);
const setSelectedRow = React.useContext(GridSelectedRowDispatchContext);

return (
<input
{...other}
type="radio"
tabIndex={-1}
id={instanceId}
tabIndex={0}
checked={row && row.id === selectedRow}
value={row ? row.id : ''}
Copy link
Contributor

Choose a reason for hiding this comment

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

Something I'm a little unsure about here is if they still pass in a value what should we do with it?

One tricky thing with adding in context is that if, for whatever reason, someone uses StructuredListInput without the wrapper components then this component ends up not working because the context is never defined.

I guess this kind of goes back to your point earlier about how this might be too big of a breaking change to safely migrate to in v10? Curious what your thoughts are

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its worth talking about for sure -- I guess what are the chances or what is the actual use case of only using StructuredListInput? Should we put a call out on Slack or maybe kick this whole can down the road to v11?

Copy link
Contributor

Choose a reason for hiding this comment

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

@dakahn maybe the safest thing is just to wrap it all behind a flag, I guess? If we want to do it in v10, then I think we'll just need to make sure we understand what props teams could be supplying and how to leverage/use them, if so.

For example, if someone is using a label then they might be expecting in a test to be able to find a node using that label. If we no longer use it in the component, then their test would be break.

onChange={(event) => {
setSelectedRow(event.target.value);
}}
id={!id && defaultId}
className={classes}
value={value}
name={name}
title={title}
/>
Expand All @@ -225,7 +251,10 @@ StructuredListInput.propTypes = {
/**
* Specify whether the underlying input should be checked by default
*/
defaultChecked: PropTypes.bool,
defaultChecked: deprecate(
PropTypes.bool,
`\nThe prop \`defaultChecked\` is no longer needed and will be removed in the next major version of Carbon.`
),

/**
* Specify a custom `id` for the input
Expand All @@ -240,7 +269,10 @@ StructuredListInput.propTypes = {
/**
* Provide an optional hook that is called each time the input is updated
*/
onChange: PropTypes.func,
onChange: deprecate(
PropTypes.func,
`\nThe prop \`onChange\` will be removed in the next major version of Carbon.`
),

/**
* Provide a `title` for the input
Expand All @@ -250,12 +282,13 @@ StructuredListInput.propTypes = {
/**
* Specify the value of the input
*/
value: PropTypes.oneOfType([PropTypes.string, PropTypes.number]).isRequired,
value: deprecate(
PropTypes.oneOfType([PropTypes.string, PropTypes.number]).isRequired,
`\nThe prop \`value\` will be removed in the next major version of Carbon.`
),
};

StructuredListInput.defaultProps = {
onChange: () => {},
value: 'value',
title: 'title',
};

Expand All @@ -267,8 +300,12 @@ export function StructuredListCell(props) {
[`${prefix}--structured-list-content--nowrap`]: noWrap,
});

return (
<div className={classes} role={head ? 'columnheader' : 'cell'} {...other}>
return head ? (
dakahn marked this conversation as resolved.
Show resolved Hide resolved
<span className={classes} role="columnheader" {...other}>
{children}
</span>
) : (
<div className={classes} role="cell" {...other}>
{children}
</div>
);
Expand Down