Add ActionList.Heading component #3581

broccolinisoup · 2023-08-01T02:46:22Z

As a part of https://github.com/github/accessibility-audits/issues/2939, we are adding ActionList.Heading component that has been proposed and discussed here Most of the content below is from the API PR - Thanks to @siddharthkp 🙌🏻

DESIRED SEMANTICS

<h3 id="heading-id" className="sx-generated">Issue actions</h2>
<ul role="list" aria-labelledby="heading-id">
  <li>
    <span><!-- leading visual --></span>
    <span>Lock conversation</span>
  </li>
  <li>
    <span><!-- leading visual --></span>
    <span>Unpin issue</span>
    <span><!-- description --></span>
  </li>
</ul>

CURRENT API

<Heading 
  as="h3"
  id="heading-id" 
  sx={{fontSize: 0, color:'fg.muted', paddingX: 3}}
>
  Issue actions
</Heading>

<ActionList role="list" aria-labelledby="heading-id">
  <ActionList.Item>
    {/* leading visual */}
    Lock conversation
  </ActionList.Item>
  <ActionList.Item>
    {/* leading visual */}
    Unpin issue
    {/* description */}
   </ActionList.Item>
</ActionList>

NEW API

<ActionList role="list">
 <ActionList.Heading as="h3">Issue actions</ActionList.Heading>
 <ActionList.Item>
    {/* leading visual */}
    Lock conversation
 </ActionList.Item>
 <ActionList.Item>
    {/* leading visual */}
    Unpin issue
    {/* description */}
 </ActionList.Item>
</ActionList>

Problems we are trying to solve:

✅ Easy to miss, to not add heading at all (Missing heading)
✅ Need to bring your own id + aria-labelledby (Heading but not connected)
✅ Need to bring your own heading styles (Inconistent heading styles)
✅ Need to bring your own padding to align with ActionList's default padding which depends on variant prop on ActionList (Inconsistent space between heading and list)

Features/extensions that we are introducing to support:

✅ Add arbitrary attributes to underlying element like id or data-test-id
✅ Make the heading visually hidden but readable by screen readers (see [Primer][React] ActionList semantics accessibility-audits#2939 (comment) thread for context)
✅ Customize heading styles (not recommended, but for exceptions)

Screenshots

Please provide before/after screenshots for any visual changes

Merge checklist

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

changeset-bot · 2023-08-01T02:46:24Z

🦋 Changeset detected

Latest commit: c68c0cc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2023-08-01T02:49:53Z

size-limit report 📦

Path	Size
dist/browser.esm.js	104.52 KB (+0.16% 🔺)
dist/browser.umd.js	105.1 KB (+0.15% 🔺)

.changeset/slimy-starfishes-agree.md

src/ActionList/Heading.tsx

src/ActionList/ActionList.features.stories.tsx

src/ActionList/Heading.tsx

siddharthkp · 2023-08-03T13:51:26Z

src/ActionList/ActionList.features.stories.tsx

@@ -41,6 +42,109 @@ export const SimpleList = () => (
  </ActionList>
 )

+export const WithHeading = () => (


I think we should add this heading to docs and all stories in another PR after this, to show that this is the recommended usage.

Oh that is right! All should have a heading.

We should also update the ActionList usages in other primer react components , right? 🤔 Like NavList to lead the change

siddharthkp · 2023-08-03T13:58:59Z

src/ActionList/List.tsx

@@ -42,32 +47,42 @@ export const List = React.forwardRef<HTMLUListElement, ActionListProps>(
      paddingY: variant === 'inset' ? 2 : 0,
    }

+    const [slots, childrenWithoutSlots] = useSlots(props.children, {
+      heading: Heading,
+    })


Would this already work with eslint-plugin-primer-react or do we need to update that as well?

I think we would need to make some update on the eslint plugin.

We need to add ActionList: ['ActionList.Header'], in this object

Although, this rule will only prevent folks from using ActionList.Heading outside of ActionList, this won't warn if the ActionList is missing ActionList.Heading which I believe this is something we need as well? Can we do this with the type system at all? 🤔 (This would make the change breaking, though) Or should we look into adding another eslint rule for that too?

Also we can add ActionList.Heading to the a11y-explicit-heading rule to restrict the values passed to as

Considering action lists in menus shouldn't have a heading (reference), maybe we can enforece this with the eslint rule as well?

Also we can add ActionList.Heading to the a11y-explicit-heading rule to restrict the values passed to as

Because ActionList.Heading is a new component, we can also use typescript to restrict allowed values for as :)

Considering action lists in menus shouldn't have a heading (#3581 (comment)), maybe we can enforece this with the eslint rule as well?

Good point. We could add an eslint rule for this, however, I'm not sure how effectively we would be able to catch incorrect usage given ActionList does not have to be a direct child of ActionMenu, it can be abstracted in a custom UserList or BranchList component that uses ActionList internally.

We also have the option of a dev environment runtime warning. Because we know when an ActionList is contained inside ActionMenu, we could use that to throw a warning

Because ActionList.Heading is a new component, we can also use typescript to restrict allowed values for as :)

That is so true! I missed that 😅 which is already restricted anyway!

We also have the option of a dev environment runtime warning. Because we know when an ActionList is contained inside ActionMenu, we could use that to throw a warning

I love this! Thanks for the context. I made it invariant to throw an error. Because first, it is a new component and it won't break anything. Second, when there is a heading the list will be labelled by the heading and completely disregard the button name. This could potentially be misleading.

All that said, if you think that is unnecessary and very little chance of creating a misleading experience, happy to make it warning as well 😊

siddharthkp

Overall, looks like solid work!

There are some tiny considerations here and there :)

siddharthkp · 2023-09-06T13:10:26Z

src/ActionList/Heading.tsx

+} & SxProp
+
+export const Heading = forwardRef(
+  ({as: Component = 'h3', children, sx = defaultSxProp, visuallyHidden = false, ...props}, forwardedRef) => {


Non blocking: Should there be a default h3 or should we always ask for the heading level by making as required without default?

Related work from eslint-plugin-primer-react:

eslint-plugin-primer-react/rules/a11y-explicit-heading

[Idea] Opinionated rule that requires as to be set for <Heading> eslint-plugin-primer-react#57

Ohh this is great! Yes I agree we should make it required!

…eading

broccolinisoup · 2023-09-27T23:08:13Z

Integration PR is all green 🎉 https://github.com/github/github/pull/290302 merging the PR

broccolinisoup temporarily deployed to github-pages August 1, 2023 02:53 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3581 August 1, 2023 02:54 Inactive

Add ActionList.Heading component

7980fe1

broccolinisoup force-pushed the bs/actionlist-heading branch from ba37bef to 7980fe1 Compare August 1, 2023 03:09

broccolinisoup commented Aug 1, 2023

View reviewed changes

.changeset/slimy-starfishes-agree.md Outdated Show resolved Hide resolved

Update .changeset/slimy-starfishes-agree.md

5f1a322

broccolinisoup commented Aug 1, 2023

View reviewed changes

src/ActionList/Heading.tsx Outdated Show resolved Hide resolved

github-actions bot temporarily deployed to storybook-preview-3581 August 1, 2023 03:14 Inactive

github-actions bot temporarily deployed to storybook-preview-3581 August 1, 2023 03:15 Inactive

broccolinisoup commented Aug 1, 2023

View reviewed changes

src/ActionList/ActionList.features.stories.tsx Outdated Show resolved Hide resolved

broccolinisoup temporarily deployed to github-pages August 1, 2023 03:18 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3581 August 1, 2023 03:19 Inactive

broccolinisoup commented Aug 1, 2023

View reviewed changes

src/ActionList/Heading.tsx Outdated Show resolved Hide resolved

broccolinisoup marked this pull request as ready for review August 1, 2023 04:16

broccolinisoup requested review from a team, josepmartins and siddharthkp August 1, 2023 04:16

siddharthkp reviewed Aug 3, 2023

View reviewed changes

heading styles

576161f

broccolinisoup temporarily deployed to github-pages August 9, 2023 01:43 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3581 August 9, 2023 01:44 Inactive

broccolinisoup added 2 commits August 31, 2023 14:36

Merge branch 'main' into bs/actionlist-heading

6b613d3

Update heading style and add visually hidden wrapper with story updates

5a316b5

broccolinisoup temporarily deployed to github-pages September 1, 2023 02:25 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3581 September 1, 2023 02:25 Inactive

siddharthkp reviewed Sep 6, 2023

View reviewed changes

github-actions bot temporarily deployed to storybook-preview-3581 September 10, 2023 22:10 Inactive

broccolinisoup temporarily deployed to github-pages September 12, 2023 01:27 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3581 September 12, 2023 01:27 Inactive

broccolinisoup force-pushed the bs/actionlist-heading branch from a985caf to 32bb440 Compare September 12, 2023 01:34

Merge branch 'main' into bs/actionlist-heading

3213b5e

github-actions bot temporarily deployed to storybook-preview-3581 September 12, 2023 01:40 Inactive

broccolinisoup temporarily deployed to github-pages September 12, 2023 01:44 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3581 September 12, 2023 01:45 Inactive

broccolinisoup temporarily deployed to github-pages September 12, 2023 02:41 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3581 September 12, 2023 02:42 Inactive

Merge branch 'main' into bs/actionlist-heading

c5d2038

broccolinisoup force-pushed the bs/actionlist-heading branch from 250f91e to c5d2038 Compare September 20, 2023 02:30

broccolinisoup temporarily deployed to github-pages September 20, 2023 02:38 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3581 September 20, 2023 02:39 Inactive

Merge branch 'main' into bs/actionlist-heading

c24a4eb

broccolinisoup temporarily deployed to github-pages September 27, 2023 01:08 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3581 September 27, 2023 01:09 Inactive

remove the full variant story because there is no full variant list h…

c68c0cc

…eading

broccolinisoup deployed to github-pages September 27, 2023 22:59 — with GitHub Actions View deployment

github-actions bot deployed to storybook-preview-3581 September 27, 2023 22:59 View deployment

broccolinisoup added this pull request to the merge queue Sep 27, 2023

Merged via the queue into main with commit cc12464 Sep 27, 2023
31 checks passed

broccolinisoup deleted the bs/actionlist-heading branch September 27, 2023 23:18

This was referenced Sep 27, 2023

Release tracking #3765

Merged

Release tracking (next major) #3147

Closed

skemex mentioned this pull request May 24, 2024

[Snyk] Upgrade @primer/react from 35.26.1 to 35.32.2 skemex/wpe-actions-workshop#45

Open

rbragaru mentioned this pull request Aug 14, 2024

[Snyk] Upgrade @primer/react from 35.15.1 to 35.32.2 rbragaru/rocketdex#4

Open

skemex mentioned this pull request Sep 24, 2024

[Snyk] Upgrade @primer/react from 35.26.1 to 35.32.2 skemex/wpe-actions-workshop#55

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ActionList.Heading component #3581

Add ActionList.Heading component #3581

broccolinisoup commented Aug 1, 2023 •

edited

Loading

changeset-bot bot commented Aug 1, 2023 •

edited

Loading

github-actions bot commented Aug 1, 2023 •

edited

Loading

siddharthkp Aug 3, 2023

broccolinisoup Aug 9, 2023 •

edited

Loading

siddharthkp Aug 3, 2023

broccolinisoup Aug 9, 2023 •

edited

Loading

broccolinisoup Sep 1, 2023

siddharthkp Sep 6, 2023

siddharthkp Sep 6, 2023

broccolinisoup Sep 7, 2023

siddharthkp left a comment

siddharthkp Sep 6, 2023

broccolinisoup Sep 7, 2023

broccolinisoup commented Sep 27, 2023

Add ActionList.Heading component #3581

Add ActionList.Heading component #3581

Conversation

broccolinisoup commented Aug 1, 2023 • edited Loading

Screenshots

Merge checklist

changeset-bot bot commented Aug 1, 2023 • edited Loading

🦋 Changeset detected

github-actions bot commented Aug 1, 2023 • edited Loading

size-limit report 📦

Choose a reason for hiding this comment

broccolinisoup Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

broccolinisoup Aug 9, 2023 • edited Loading

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

siddharthkp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

broccolinisoup commented Sep 27, 2023

broccolinisoup commented Aug 1, 2023 •

edited

Loading

changeset-bot bot commented Aug 1, 2023 •

edited

Loading

github-actions bot commented Aug 1, 2023 •

edited

Loading

broccolinisoup Aug 9, 2023 •

edited

Loading

broccolinisoup Aug 9, 2023 •

edited

Loading