feat(SimpleList): add simple list #3645

kmcfaul · 2020-02-03T19:38:08Z

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.

patternfly-build · 2020-02-03T19:41:50Z

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

mcoker · 2020-02-03T20:08:31Z

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.

mcarrano

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

kmcfaul · 2020-02-04T18:36:34Z

@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-io · 2020-02-04T18:43:37Z

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.

mcoker

lgtm, thanks!!

dlabrecq · 2020-02-05T16:03:14Z

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

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


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?

mcarrano

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

mcoker · 2020-02-05T19:09:51Z

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

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

mcoker · 2020-02-08T01:14:05Z

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

kmcfaul · 2020-02-10T17:03:27Z

@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?

dlabrecq

Thanks for adding SimpleListItemProps

packages/patternfly-4/react-core/src/components/SimpleList/SimpleList.tsx

jschuler · 2020-02-12T20:10:58Z

packages/patternfly-4/react-core/src/components/SimpleList/SimpleListGroup.tsx

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


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

packages/patternfly-4/react-core/src/components/SimpleList/SimpleListItem.tsx

packages/patternfly-4/react-core/src/components/SimpleList/SimpleList.tsx

packages/patternfly-4/react-core/src/components/SimpleList/SimpleListGroup.tsx

jschuler · 2020-02-12T20:19:07Z

packages/patternfly-4/react-core/src/components/SimpleList/SimpleListItem.tsx

+
+          const componentProps = isButton
+            ? {
+                type


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

jschuler · 2020-02-12T20:19:35Z

packages/patternfly-4/react-core/src/components/SimpleList/SimpleListItem.tsx

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


className should be on the root element

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

jessiehuff

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! :)

jessiehuff

LGTM! :)

mattnolting

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>

mattnolting · 2020-02-13T17:43:37Z

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 😄

mcarrano

This looks great!

mattnolting · 2020-02-13T18:20:05Z

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".

mattnolting

🔥 LGTM! Nice work!

patternfly-build · 2020-02-14T15:29:58Z

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 PF4 React issues for the PF4 core effort ux review labels Feb 3, 2020

kmcfaul requested review from tlabaj, mcarrano and mcoker 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 requested changes Feb 5, 2020

View reviewed changes

mcarrano requested changes Feb 5, 2020

View reviewed changes

jschuler reviewed Feb 5, 2020

View reviewed changes

packages/patternfly-4/react-core/src/experimental/components/SimpleList/SimpleList.test.tsx Outdated Show resolved Hide resolved

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 requested changes Feb 12, 2020

View reviewed changes

feat(SimpleList): add pr feedback

ff94d3b

jessiehuff requested 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(SimpleList): add simple list #3645

feat(SimpleList): add simple list #3645

kmcfaul commented Feb 3, 2020

patternfly-build commented Feb 3, 2020 •

edited

mcoker commented Feb 3, 2020

mcarrano left a comment

kmcfaul commented Feb 4, 2020

codecov-io commented Feb 4, 2020 •

edited

mcoker left a comment

dlabrecq Feb 5, 2020

mcarrano left a comment

mcoker commented Feb 5, 2020

mcoker commented Feb 8, 2020

kmcfaul commented Feb 10, 2020

dlabrecq left a comment

jschuler Feb 12, 2020

kmcfaul Feb 12, 2020

jschuler Feb 12, 2020

jschuler Feb 12, 2020

kmcfaul Feb 12, 2020

kmcfaul Feb 12, 2020

jschuler Feb 12, 2020

kmcfaul Feb 12, 2020

jessiehuff left a comment

jessiehuff left a comment

mattnolting left a comment •

edited

mattnolting commented Feb 13, 2020

mcarrano left a comment

mattnolting commented Feb 13, 2020

mattnolting left a comment

patternfly-build commented Feb 14, 2020

feat(SimpleList): add simple list #3645

feat(SimpleList): add simple list #3645

Conversation

kmcfaul commented Feb 3, 2020

patternfly-build commented Feb 3, 2020 • edited

mcoker commented Feb 3, 2020

mcarrano left a comment

Choose a reason for hiding this comment

kmcfaul commented Feb 4, 2020

codecov-io commented Feb 4, 2020 • edited

Codecov Report

mcoker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcarrano left a comment

Choose a reason for hiding this comment

mcoker commented Feb 5, 2020

mcoker commented Feb 8, 2020

kmcfaul commented Feb 10, 2020

dlabrecq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessiehuff left a comment

Choose a reason for hiding this comment

jessiehuff left a comment

Choose a reason for hiding this comment

mattnolting left a comment • edited

Choose a reason for hiding this comment

mattnolting commented Feb 13, 2020

mcarrano left a comment

Choose a reason for hiding this comment

mattnolting commented Feb 13, 2020

mattnolting left a comment

Choose a reason for hiding this comment

patternfly-build commented Feb 14, 2020

patternfly-build commented Feb 3, 2020 •

edited

codecov-io commented Feb 4, 2020 •

edited

mattnolting left a comment •

edited