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.

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

Jump to bottom

Components: List component that ensures that the role is properly handled #19061

Closed
wants to merge 1 commit into from
Closed

Components: List component that ensures that the role is properly handled #19061

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 5 additions & 3 deletions packages/components/src/custom-select-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@ import classnames from 'classnames';
/**
* Internal dependencies
*/
import { Button, Dashicon } from '../';
import Button from '../button';
import Dashicon from '../dashicon';
import List from '../list';

const itemToString = ( item ) => item && item.name;
// This is needed so that in Windows, where
Expand Down Expand Up @@ -107,7 +109,7 @@ export default function CustomSelectControl( {
className="components-custom-select-control__button-icon"
/>
</Button>
<ul { ...menuProps }>
<List { ...menuProps }>
{ isOpen &&
items.map( ( item, index ) => (
// eslint-disable-next-line react/jsx-key
Expand All @@ -134,7 +136,7 @@ export default function CustomSelectControl( {
{ item.name }
</li>
) ) }
</ul>
</List>
</div>
);
}
13 changes: 7 additions & 6 deletions packages/components/src/form-token-field/suggestions-list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ import classnames from 'classnames';
import { Component } from '@wordpress/element';
import { withSafeTimeout } from '@wordpress/compose';

/**
* Internal dependencies
*/
import List from '../list';

class SuggestionsList extends Component {
constructor() {
super( ...arguments );
Expand Down Expand Up @@ -73,12 +78,8 @@ class SuggestionsList extends Component {
}

render() {
// We set `tabIndex` here because otherwise Firefox sets focus on this
// div when tabbing off of the input in `TokenField` -- not really sure
// why, since usually a div isn't focusable by default
// TODO does this still apply now that it's a <ul> and not a <div>?
return (
<ul
<List
ref={ this.bindList }
className="components-form-token-field__suggestions-list"
id={ `components-form-token-suggestions-${ this.props.instanceId }` }
Expand Down Expand Up @@ -120,7 +121,7 @@ class SuggestionsList extends Component {
/* eslint-enable jsx-a11y/click-events-have-key-events */
} )
}
</ul>
</List>
);
}
}
Expand Down
8 changes: 6 additions & 2 deletions packages/components/src/guide/page-control.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,15 @@ import { __, sprintf } from '@wordpress/i18n';
* Internal dependencies
*/
import IconButton from '../icon-button';
import List from '../list';
import { PageControlIcon } from './icons';

export default function PageControl( { currentPage, numberOfPages, setCurrentPage } ) {
return (
<ul className="components-guide__page-control" aria-label={ __( 'Guide controls' ) }>
<List
className="components-guide__page-control"
accessibilityLabel={ __( 'Guide controls' ) }
>
{ times( numberOfPages, ( page ) => (
<li key={ page }>
<IconButton
Expand All @@ -28,6 +32,6 @@ export default function PageControl( { currentPage, numberOfPages, setCurrentPag
/>
</li>
) ) }
</ul>
</List>
);
}
16 changes: 16 additions & 0 deletions packages/components/src/list/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/**
* WordPress dependencies
*/
import { createElement, forwardRef } from '@wordpress/element';

const List = ( { type = 'unordered', accessibilityLabel, ...props }, ref ) => {
const tagName = type === 'ordered' ? 'ol' : 'ul';
Copy link
Member

Choose a reason for hiding this comment

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

If it's a "one or the other" decision, I think a boolean value would be preferable here, e.g. isOrdered. These sorts of magic strings could be difficult to work with.

Copy link
Member Author

Choose a reason for hiding this comment

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

A question I had coming into this is specifics around handling different types of list (ol, ul, maybe dl?), and the relationship between the parent element and the child element (li,dd,dt). Should we want to complete this abstraction with a corresponding ListItem element as well?

The answer is highly coupled with what you shared in the comment cited. My understanding was that we could support multiple types:

  • unordered - ul
  • ordered - ol
  • description - dl
  • unstyled - I suppose ul(?) but all styles removed

With that, it would be great to have ListItem component which would handle tag names dance internally (li for ul and ol, dd and dt for dl). In addition, it would make it possible to inherit those types for nested lists from the parents and so on. I'm sure there would be more benefits discovered.

return createElement( tagName, {
role: 'list',
'aria-label': accessibilityLabel,
...props,
ref,
} );
};

export default forwardRef( List );