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

fix: list selection behavior + accessibility #1167

Merged
merged 25 commits into from
Aug 18, 2020
Merged

Conversation

prsdthkr
Copy link
Member

@prsdthkr prsdthkr commented Aug 13, 2020

Description

In this change, we

  • add selected property for List.Selection to allow setting the checked state of the checkbox. This property will be internally set by the parent List.Item component based on the List.Item.selected property.
  • add a selectable property to List to
    • add fd-list--selection class to <ul>
    • set <ul> role='listbox'
  • remove List.Header and List.Footer from the list structure, they should be passed through the new header and footer List props.
  • List.Header renders <h2-6> depending on List.headerLevel
  • List.Footer renders <span>
  • add an id property to List to associate List.Header as label
  • add checkBoxAriaLabel to List.Selection for accessibility
  • add headerClassName and footerClassName props to allow custom classes

Screen cap

Before
list-selection-bug

After
list-selection-fixed

Know issues

  • deviates from fundamental-styles markup for accessibility reasons

fixes #1164 #970

@prsdthkr prsdthkr added bug Something isn't working / Issues in the code A11y Accessiblity labels Aug 13, 2020
@prsdthkr prsdthkr self-assigned this Aug 13, 2020
@netlify
Copy link

netlify bot commented Aug 13, 2020

Deploy preview for fundamental-react ready!

Built with commit 0c28fd9

https://deploy-preview-1167--fundamental-react.netlify.app

@prsdthkr prsdthkr marked this pull request as ready for review August 13, 2020 23:12
@prsdthkr prsdthkr requested a review from a team August 13, 2020 23:13
src/List/List.js Outdated
{listHeader && React.cloneElement(listHeader, { id: `${listId}-label` })}
<ul
{...props}
{...(selectable ? { 'role': 'listbox' } : null)}
Copy link
Contributor

@skvale skvale Aug 14, 2020

Choose a reason for hiding this comment

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

Slightly easier to read syntax:

role={selectable ? 'listbox' : undefined}

src/List/List.js Outdated
{...props}
{...(selectable ? { 'role': 'listbox' } : null)}
aria-labelledby={listHeader ? `${listId}-label` : null}
className={ListClasses}
Copy link
Contributor

Choose a reason for hiding this comment

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

quick unrelated change ListClasses -> listClasses

src/List/List.js Outdated
id={`${listId}-list`}
ref={ref}>
{ React.Children.map(children, child => {
if (!(isHeader(child) || isFooter(child)) ) {
Copy link
Contributor

@skvale skvale Aug 14, 2020

Choose a reason for hiding this comment

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

probably a nitpick

if (!(isHeader(child) || isFooter(child)) ) {
vs
if (!isHeader(child) && !isFooter(child)) {

But it brings up a bigger issue to me, we aren't supporting custom components or fragments. We should be able to consume this code and add the right props to the right children

const MyOtherItems = () => (
   <React.Fragment>
        <List.Item>
            <List.Text>List Item 2</List.Text>
        </List.Item>
        <List.Item>
            <List.Text>List Item 3</List.Text>
        </List.Item>
        <List.Item>
            <List.Text>List Item 4</List.Text>
        </List.Item>
   </React.Fragment>
);
// ....
const MyListHeader = () => <List.Header>List Header</List.Header>
// ...
<List>
        <MyListHeader />
        <List.Item>
            <List.Text>List Item 1</List.Text>
        </List.Item>
        <MyOtherItems />
        <List.Footer>List Footer</List.Footer>
    </List>

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for pointing this out. I removed the children scanning and made the header and footer as List props.

Copy link
Contributor

@skvale skvale Aug 14, 2020

Choose a reason for hiding this comment

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

That's an interesting idea to get around the problem. I think we might want it to look more like this though

<List
  footer='My footer'
  header='My header'>
   // ...

and the List component would render them like

return (
  <>
    {header && <List.Header id={`${listId}-label`}>{header}</List.Header>}
    // ...
    {footer && <List.Footer>{footer}</List.Footer>}
  </>


footer: PropTypes.node,
header: PropTypes.node,

Copy link
Member Author

Choose a reason for hiding this comment

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

This will mean that we cannot get fancy with the header and footer like here, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it would look like

footer = {<React.Fragment><List.Text><b>Total</b></List.Text><List.Text secondary>7</List.Text></React.Fragment>}

instead of

        footer={
            <List.Footer>
                <List.Text><b>Total</b></List.Text>
                <List.Text secondary>7</List.Text>
            </List.Footer>
        }

@prsdthkr prsdthkr requested a review from skvale August 17, 2020 19:38
src/List/List.js Outdated
/** Set to **true** if any list item has a byline. */
hasByline: PropTypes.bool,
/** The list header as a String or a React component.*/
header: PropTypes.oneOfType([PropTypes.string, PropTypes.object]),
Copy link
Contributor

Choose a reason for hiding this comment

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

React component
PropTypes.object

It's more accurate to say React node and PropTypes.node

@@ -13,9 +13,9 @@ const ListFooter = ({
);

return (
<li {...props} className={ListItemClasses}>
<span {...props} className={ListItemClasses}>
Copy link
Contributor

@skvale skvale Aug 17, 2020

Choose a reason for hiding this comment

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

This seems right to change the footer to a span, but do we need to stay in line with fundamental-styles here?

I've looked at a few of their examples and they all still show the footer as a li, like here: https://github.com/SAP/fundamental-styles/blob/master/stories/list/standard/standard-list.stories.js#L301

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is part of

deviates from fundamental-styles markup for accessibility reasons

from your description, but is that OK to do? Do we document our deviations?

Copy link
Member Author

@prsdthkr prsdthkr Aug 17, 2020

Choose a reason for hiding this comment

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

Trying to get Got this change in fundamental-styles: SAP/fundamental-styles#1397

We can either jump the gun and get this change in fd-react or wait for the fd-styles change and subsequent fd-styles upgrade in fd-react.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm for jumping the gun, as we will eventually be up to spec when fd-styles is upgraded :)

@prsdthkr prsdthkr requested a review from skvale August 18, 2020 21:08
@prsdthkr prsdthkr merged commit 2c4cbed into master Aug 18, 2020
@prsdthkr prsdthkr linked an issue Aug 19, 2020 that may be closed by this pull request
@prsdthkr prsdthkr deleted the fix/list-item-selection branch September 15, 2020 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A11y Accessiblity bug Something isn't working / Issues in the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

List: selected state for List.Selection ListItem "selected" prop not working
2 participants