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

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

Closed
wants to merge 1 commit into from

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Dec 11, 2019

Description

This PR proposes that we introduce List component which will handle some accessibility concerns behind the scenes. It's quite common that the following code needs to be written:

/*
* Disable reason: The `list` ARIA role is redundant but
* Safari+VoiceOver won't announce the list otherwise.
*/
/* eslint-disable jsx-a11y/no-redundant-roles */
<ul role="list" className="block-directory-downloadable-blocks-list">

You can find it in a few places. In other places, it's probably missing. The idea is to have a common abstraction that automates it.

We would have to expose List as part of public API and discourage direct usage of ul and ol tags as components. It can be implemented similar to how button or svg tags are detected by ESLint:

gutenberg/.eslintrc.js

Lines 130 to 145 in ee3e0b2

'react/forbid-elements': [ 'error', {
forbid: [
[ 'button', 'Button' ],
[ 'circle', 'Circle' ],
[ 'g', 'G' ],
[ 'path', 'Path' ],
[ 'polygon', 'Polygon' ],
[ 'rect', 'Rect' ],
[ 'svg', 'SVG' ],
].map( ( [ element, componentName ] ) => {
return {
element,
message: `use cross-platform <${ componentName } /> component instead.`,
};
} ),
} ],

This will make it easier to write cross-platform lists in the long run.

How has this been tested?

npm run test-e2e
npm run test-unit

Types of changes

Refactoring, no visual changes.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

@gziolo gziolo added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible labels Dec 11, 2019
@gziolo gziolo added the [Package] Components /packages/components label Dec 11, 2019
@gziolo gziolo self-assigned this Dec 11, 2019
@ItsJonQ
Copy link

ItsJonQ commented Dec 11, 2019

@gziolo Awesome! I think it's great that we're adding more primitives to @wordpress/components! Having them take care of a11y concerns is a great built-in feature.

What are your thoughts in regards to styling 😊?

I believe the most common styling related patterns for lists would be in regards to alignment (vertically stacked, or horizontally inline), the spacing between list items (margins), and perhaps the bullet UI (styled or unstyled). Perhaps exposing these adjustments as props, making it easier to do ad-hoc adjustments via the component rather than adding custom CSS.

The above may be out of scope for this PR. I just wanted to get your thoughts! If you think it is a good idea, maybe we can follow it up with a PR to introduce list styles.

Thanks again!! <3

@skorasaurus
Copy link
Member

skorasaurus commented Dec 11, 2019

Yep, as @ItsJonQ mentioned, This might be out of scope of the PR (I'm fairly basic at react) but I'm picking up work to create additional ordinal list styles (#13888) and am wondering if the ability to add additional list styles would be able to be built off of this.

Some other issues that include discussion of it is #12420 and #17738

EDIT: (Re-reading over https://developer.wordpress.org/block-editor/packages/packages-components/ ; I'm beginning to understand that the component may only be for the Gutenberg interface itself, not for the contents and styling of blocks that we make, so if this interpretation is correct, then disregard this :))

@aduth
Copy link
Member

aduth commented Dec 11, 2019

At least for posterity's sake, it's worth noting that in the original conversation (link requires registration), I pointed out that we could alternatively enforce the use of the role attribute via a custom ESLint rule. The technical difference here is minimal, and to me the bigger concern is discoverability, usability, and perhaps future flexibility as well. Would one or the other of a List component or ESLint rule be more obvious to a developer in using? Do we foresee that we'd use this List component for anything other than just adding this role?

One nice thing about offering this as a component is that we might have a better opportunity to thoroughly document its purpose, moreso than I think ESLint rules can express. We should want to make sure this is included before merging, if we go with this approach.

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?

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.

@gziolo
Copy link
Member Author

gziolo commented Dec 12, 2019

I believe the most common styling related patterns for lists would be in regards to alignment (vertically stacked, or horizontally inline), the spacing between list items (margins), and perhaps the bullet UI (styled or unstyled). Perhaps exposing these adjustments as props, making it easier to do ad-hoc adjustments via the component rather than adding custom CSS.

I didn't look at styles at all, I focused on the accessibility aspect of it and the inconvenience this role attribute and corresponding code comment create for developers and reviewers. I wanted to keep this PR small, but I think we should definitely iterate and introduce the pair of List and ListItem components that can work together. I shared some more ideas in #19061 (comment).

Some other issues that include discussion of it is #12420 and #17738

I wanted to make it clear that this PR is unrelated to the List block. It's about introducing some basic component(s) to use when using lists in the block editor's UI.

Would one or the other of a List component or ESLint rule be more obvious to a developer in using? Do we foresee that we'd use this List component for anything other than just adding this role?

I shared some ideas in #19061 (comment).

One nice thing about offering this as a component is that we might have a better opportunity to thoroughly document its purpose, moreso than I think ESLint rules can express. We should want to make sure this is included before merging, if we go with this approach.

Yes, it's also a good opportunity to have more control over how the lists are handled in Gutenberg. In general, I feel like we could benefit from all those abstractions to unify UI and provide some basic styling and behavior which gives coherent experience.

@gziolo
Copy link
Member Author

gziolo commented Jun 17, 2020

I'm closing it for now as I don't have time to work on it but I plan to revisit it one day :)

@gziolo gziolo closed this Jun 17, 2020
@gziolo gziolo deleted the update/list-component branch June 17, 2020 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants