feat(SimpleList): add simple list by kmcfaul · Pull Request #3645 · patternfly/patternfly-react · GitHub

What: Closes #3610

@mcoker It appears that pf-c-simple-list__list and pf-c-simple-list__list-item do not exist/are not in use despite appearing on the HTML structure in core. Let me know if I should add them manually, but currently I have left them off.

PF3 preview: https://patternfly-react-pr-3645-pf3.surge.sh
PF4 preview: https://patternfly-react-pr-3645.surge.sh
A11y report: https://patternfly-react-pr-3645-coverage.surge.sh

I wonder if we should add an id to the __title, then aria-labelledby to the __section to match the nav component? Also curious if we should make that update to the dropdown, app launcher, options menu, etc?
We probably want to add type="button" to the buttons so that they aren't submit buttons.
This is a bug in core, but we need to add text-decoration: none to the <a> items so that they don't get an underline on hover.
Add an href attribute to the <a> __item-link elements so that they're in the tabindex. Or maybe add tabindex="0" to elements that aren't a button, or just all for consistency?

cc @jessiehuff

It appears that pf-c-simple-list__list and pf-c-simple-list__list-item do not exist/are not in use despite appearing on the HTML structure in core. Let me know if I should add them manually, but currently I have left them off.

I'm curious what @tlabaj thinks about this. I'm not sure that we have a consistent way of doing this, and it often comes up in the PR when we give an element a class that doesn't have a corresponding style.

The main reason we do it is that it helps build the BEM structure (creates a namespace for elements), and allows us to style those elements at some later point without requiring users update their markup to add a new class. That mostly applies to core users, since we can always add a missing class to a react component after the fact.

This looks good to me. @lboehling acn you also take a look?

@mcoker Added your bullets (minus the core one). I went with adding the tabindex="0" only to the anchor elements.

@jessiehuff Let me know if I'm missing any other aria labels or tags that should be explicitly defined. I think in most cases using the ...props to pass these in should work.

Codecov Report

Merging #3645 into master will increase coverage by 0.07%.
The diff coverage is 91.89%.

@@            Coverage Diff             @@
##           master    #3645      +/-   ##
==========================================
+ Coverage   71.22%   71.29%   +0.07%     
==========================================
  Files         779      782       +3     
  Lines       10491    10528      +37     
  Branches     2263     2277      +14     
==========================================
+ Hits         7472     7506      +34     
- Misses       2592     2594       +2     
- Partials      427      428       +1

Flag	Coverage Δ
#misc	95.45% <ø> (ø)	⬆️
#patternfly3	85.89% <ø> (ø)	⬆️
#patternfly4	60.32% <91.89%> (+0.19%)	⬆️

Impacted Files	Coverage Δ
...-core/src/components/SimpleList/SimpleListItem.tsx	100% <100%> (ø)
...core/src/components/SimpleList/SimpleListGroup.tsx	71.42% <71.42%> (ø)
...eact-core/src/components/SimpleList/SimpleList.tsx	94.73% <94.73%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eab8040...c0174ee. Read the comment docs.

lgtm, thanks!!

Instead of providing the button associated with clicking on each menu option, I'd prefer to provide my own ID. I can get the label from the button, but it's difficult to work with considering the label may be localized.

...

@dlabaj I believe we may be using SelectOption to help with this?

@kmcfaul some of the list items seem to be showing an underline on_hover. See below. Why is that?

@mcarrano that's a bug from core. Just put up a PR to fix patternfly/patternfly#2679****

@kmcfaul the underline is fixed patternfly/patternfly#2679 (v2.59.1)

@dlabrecq
I added another property to the callback function that will pass back the item's props. This will work for id and the like without any special declaration.

However, I could instead add another property to SimpleListItem that would be used to pass back data, instead of props. It would be clearer what gets passed back using this approach but would require extra user input to define the structure.

Any thoughts about which approach would be better?

Thanks for adding SimpleListItemProps

should extend HTMLProps for section

I could only find React.HTMLProps<HTMLTableSectionElement>, would this be correct? It seems to work with the console logging warnings

hm, how about passing <any> in this case since we just want the default HTML attributes

just making sure this is intended, but type can be submit or reset as well if that's what's passed in as prop

we want to make sure the default type is "button", this line lets the user change that if they wish. otherwise adding a type property manually would be caught by the component and not actually set.

I'm not sure either way if we want to allow this

@mcarrano

className should be on the root element

Moved these (className, props) to root element and added properties to customize the inner component.

From an accessibility standpoint, it looks pretty good. Similar to what Coker was saying earlier, it would be beneficial to add an aria-labelledby. However, based on current recommendations, it looks like it should be added to the <ul>s though, for example here:

This aids the screen reader by being more descriptive about what that list is describing. In VO it will read "list, Group 1, 4 items" instead of "list 4 items" and not telling you which group you're under.

In terms of this pattern in other components like dropdown, app launcher, options menu, etc, I think more research and discussion would be beneficial. It can be a fine line between being descriptive and overusing aria which makes those decisions more challenging.

Great job, Katie! :)

LGTM! :)

As a followup to @jessiehuff's and @mcoker's comments and based on my VO testing, there are a couple of things you can improve here and that we need to improve upstream.

Add aria-labe="Grouped list example"l to parent list
Add role="list" to parent list .pf-c-simple-list
Add aria-hidden="true" to titles

Adding these attributes will announce that the parent .pf-c-simple-list is a list, how many sub-lists it contains and will remove titles from tabindex as they are announced when list is focused.

<div class="pf-c-simple-list" role="list" aria-label="Grouped list example">
  <section class="pf-c-simple-list__section">
    <h2 id="group-title-1" class="pf-c-simple-list__title" aria-hidden="true">Systems that are neat</h2>
    <ul class="" aria-labelledby="group-title-1">
      <li class="pf-c-simple-list__item">
        <button class="pf-c-simple-list__item-link pf-m-current" type="button">List item 1</button>
      </li>
      <li class="pf-c-simple-list__item">
        <button class="pf-c-simple-list__item-link" type="button">List item 2</button>
      </li>
      <li class="pf-c-simple-list__item">
        <button class="pf-c-simple-list__item-link" type="button">List item 3</button>
      </li>
      <li class="pf-c-simple-list__item">
        <button class="pf-c-simple-list__item-link" type="button">List item 4</button>
      </li>
    </ul>
  </section>
  <section class="pf-c-simple-list__section">
    <h2 id="group-title-1 " class="pf-c-simple-list__title" aria-hidden="true">Group 2</h2>
    <ul class="" aria-labelledby="group-2">
      <li class="pf-c-simple-list__item">
        <button class="pf-c-simple-list__item-link" type="button">List item 1</button>
      </li>
      <li class="pf-c-simple-list__item">
        <a class="pf-c-simple-list__item-link" href="#">List item 2</a>
      </li>
      <li class="pf-c-simple-list__item">
        <a class="pf-c-simple-list__item-link" href="#">List item 3</a>
      </li>
      <li class="pf-c-simple-list__item">
        <button class="pf-c-simple-list__item-link" type="button">List item 4</button>
      </li>
    </ul>
  </section>
</div>

I would also really like to see the component selectors manually added to the <li>s, but that's a larger discussion and not a blocker 😄

This looks great!

Really great work @kmcfaul ! Only one more thing for me:

If .pf-c-simple-list doesn't contain child .pf-c-simple-lists, you don't need role="list".

🔥 LGTM! Nice work!

Your changes have been released in:

@patternfly/react-catalog-view-extension@1.3.22
@patternfly/react-core@3.139.0
@patternfly/react-docs@4.19.4
@patternfly/react-inline-edit-extension@2.16.21
demo-app-ts@3.23.0
@patternfly/react-integration@3.23.0
@patternfly/react-table@2.26.21
@patternfly/react-topology@2.13.21
@patternfly/react-virtualized-extension@1.3.116

Thanks for your contribution! 🎉

kmcfaul added css review labels Feb 3, 2020

kmcfaul requested review from mcarrano, mcoker and tlabaj February 3, 2020 19:38

mcarrano reviewed Feb 3, 2020

View reviewed changes

kmcfaul requested review from jessiehuff and mcarrano February 4, 2020 18:19

dlabaj assigned dlabaj and unassigned dlabaj Feb 4, 2020

mcoker mentioned this pull request Feb 4, 2020

Simple List - follow ups from react implementation patternfly/patternfly#2674

Closed

mcoker previously approved these changes Feb 4, 2020

View reviewed changes

dlabrecq suggested changes Feb 5, 2020

View reviewed changes

mcarrano suggested changes Feb 5, 2020

View reviewed changes

jschuler reviewed Feb 5, 2020

View reviewed changes

Comment thread packages/patternfly-4/react-core/src/experimental/components/SimpleList/SimpleList.test.tsx

kmcfaul dismissed mcoker’s stale review via 51b600b February 10, 2020 16:58

kmcfaul force-pushed the simple-list branch from f61dcbc to 51b600b Compare February 10, 2020 16:58

kmcfaul force-pushed the simple-list branch from 416b351 to 702658f Compare February 12, 2020 15:45

dlabrecq previously approved these changes Feb 12, 2020

View reviewed changes

kmcfaul added 3 commits February 12, 2020 14:50

feat(SimpleList): add simple list

f9fb57f

feat(SimpleList): track current item

73a8be0

feat(SimpleList): refine features, add onSelect, add tests & integration

7a15142

kmcfaul added 2 commits February 12, 2020 14:50

feat(SimpleList): move simple list to components, rebase with master

1acffc4

fix(misc- SimpleList, FilterTableDemo): update imports for docs

fd13d0c

kmcfaul dismissed dlabrecq’s stale review via fd13d0c February 12, 2020 19:51

kmcfaul force-pushed the simple-list branch from 702658f to fd13d0c Compare February 12, 2020 19:51

jschuler suggested changes Feb 12, 2020

View reviewed changes

feat(SimpleList): add pr feedback

ff94d3b

jessiehuff suggested changes Feb 12, 2020

View reviewed changes

feat(SimpleList): move aria-labelledby to ul

c3131a6

jschuler previously approved these changes Feb 13, 2020

View reviewed changes

jessiehuff previously approved these changes Feb 13, 2020

View reviewed changes

dlabrecq previously approved these changes Feb 13, 2020

View reviewed changes

mattnolting reviewed Feb 13, 2020

View reviewed changes

feat(SimpleList): add pr feedback, aria tags

6ee92df

kmcfaul dismissed stale reviews from dlabrecq, jessiehuff, and jschuler via 6ee92df February 13, 2020 17:53

mcarrano previously approved these changes Feb 13, 2020

View reviewed changes

mcarrano added ux approved and removed ux review labels Feb 13, 2020

feat(SimpleList): assign role=list only when grouped

c0174ee

kmcfaul dismissed mcarrano’s stale review via c0174ee February 13, 2020 18:25

mattnolting approved these changes Feb 13, 2020

View reviewed changes

mattnolting mentioned this pull request Feb 13, 2020

Simple list accessibility enhancements patternfly/patternfly#2711

Closed

dlabrecq approved these changes Feb 14, 2020

View reviewed changes

jschuler approved these changes Feb 14, 2020

View reviewed changes

jschuler merged commit 4bba16c into patternfly:master Feb 14, 2020

kmcfaul mentioned this pull request Feb 14, 2020

Release notes 2020.02 #3752

Merged

+                /** Additional classes added to the SimpleList <ul> */
+                className?: string;
+                /** Callback when an item is selected */
+                onSelect?: (ref: React.RefObject<HTMLButtonElement> | React.RefObject<HTMLAnchorElement>) => void;

+                        return (
+                          <li className={css(listItemClassName)}>
+                            <Component
+                              className={css(styles.simpleListItemLink, isCurrentItem && styles.modifiers.current, className)}

+              import { css } from '@patternfly/react-styles';
+              import styles from '@patternfly/react-styles/css/components/SimpleList/simple-list';
+              export interface SimpleListGroupProps {

+                        const componentProps = isButton
+                          ? {
+                              type

feat(SimpleList): add simple list by kmcfaul · Pull Request #3645 · patternfly/patternfly-react · GitHub

Uh oh!

feat(SimpleList): add simple list - #3645

feat(SimpleList): add simple list#3645jschuler merged 12 commits intopatternfly:masterpatternfly/patternfly-react:masterfrom kmcfaul:simple-listkmcfaul/patternfly-react:simple-listCopy head branch name to clipboard

Conversation

kmcfaul commented Feb 3, 2020

Uh oh!

patternfly-build commented Feb 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcoker commented Feb 3, 2020

Uh oh!

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

kmcfaul commented Feb 4, 2020

Uh oh!

codecov-io commented Feb 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

mcoker commented Feb 5, 2020

Uh oh!

Uh oh!

mcoker commented Feb 8, 2020

Uh oh!

kmcfaul commented Feb 10, 2020

Uh oh!

dlabrecq left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jessiehuff left a comment

Choose a reason for hiding this comment

Uh oh!

jessiehuff left a comment

Choose a reason for hiding this comment

Uh oh!

mattnolting left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattnolting commented Feb 13, 2020

Uh oh!

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

mattnolting commented Feb 13, 2020

Uh oh!

mattnolting left a comment

Choose a reason for hiding this comment

Uh oh!

patternfly-build commented Feb 14, 2020

feat(SimpleList): add simple list#3645
jschuler merged 12 commits into
patternfly:masterfrom
kmcfaul:simple-list

patternfly-build commented Feb 3, 2020 •
edited
Loading

codecov-io commented Feb 4, 2020 •
edited
Loading

mattnolting left a comment •
edited
Loading