ActionList: Add ActionList.GroupHeading component and update the existing internal Header component for better semantics

[broccolinisoup]

Closes #

Changelog

New

As accepted in https://github.com/github/primer/pull/2484, added ActionList.GroupHeading API to be able to generate more accessible semantics for the action list.

Changed

Internal component

Header component in ActionList/Group.tsx renamed to be GroupHeading and exported to be used for the new ActionList.GroupHeading API but still supports the old API<ActionList.Group title="group title" >...</ActionList.Group>

Generated HTML for action list

for list role

<h2 id="list-id">Filter By</h2>
<ul role="list" aria-labelledby="list-id">
  <li>
-  <div role="presentation" aria-hidden="true"><span id="group-heading-path">Path</span></div>
+  <h3 id="group-heading-path">Path</h3>
    <ul aria-labelledby="group-heading-path">
      <li>app/assets/modules</li>
      <li>src/react/components</li>
      <li>memex/shared-ui/components</li>
    </ul>
  </li>
 <li>
    <h3 id="group-heading-advanced">Advanced</h3>
    <ul aria-labelledby="group-heading-advanced">
      <li>Owner</li>
      <li>Symbol</li>
      <li>Exclude archived</li>
    </ul>
  </li>
</ul>

For listbox and menu role

<h2 id="list-id">Reviewers</h2>
<ul role="listbox" aria-labelledby="list-id">
  <li role="none">
     <div role="presentation" aria-hidden="true"><span id="group-heading-everyone">Everyone</span></div>
-    <ul aria-labelledby="group-heading-everyone" role="group">
+    <ul aria-label="Everyone" role="group"> 
      <li role="option">broccolinisoup</li>
      <li role="option">siddharthkp</li>
      <li role="option">pksjce</li>
    </ul>
  </li>
</ul>

Removed

Rollout strategy

• Patch release
• Minor release
• Major release; if selected, include a written rollout or migration plan

Testing & Reviewing

Merge checklist

• Added/updated tests
• Added/updated documentation
• Added/updated previews (Storybook)
• Changes are SSR compatible
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge
• Integration tests pass at github/github (Learn more about how to run integration tests)

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]

🦋 Changeset detected

Latest commit: 11daf88

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]

size-limit report 📦

Path	Size
dist/browser.esm.js	104.2 KB (+0.17% 🔺)
dist/browser.umd.js	104.74 KB (+0.18% 🔺)

[siddharthkp]

GroupHeading looks great!

[joshblack]

Looking great! Just left a couple comments/questions, can't wait to hear what you think 👀

[joshblack]

From a documentation perspective, would it help to have the story name be more like ListWithHeading or something similar? Trying to think of when someone might be looking for how to do X (where X is adding a heading for a list) and having a story name that makes sense to them.

The terminology "NewGroupedHeadingAPI" feels very clear for maintainers but from a user perspective might not make as much sense.

[broccolinisoup]

Oh sorry I forgot to mention that I added these stories for dev/code review only to make the changes easily viewable. Since we are going to deprecate the title prop on ActionList.Group and our new API for providing group title is ActionList.GroupHeading, I was thinking to update the stories that use title prop with ActionList.GroupHeading such as GroupWithFilledTitle and GroupWithSubtleTitle. Would that be helpful and enough you think for consumers to find how to add group headings? Also do you think we should still keep stories with the title prop at all? 🤔

[broccolinisoup]

I'm going to take these stories back in this PR. I am thinking to do a follow up one that adds a @deprecated notation to the title prop and update all title usages with the new API in stories and docs. Let me know if there is any concern!

[joshblack]

I'm not sure what our conventions are, but when would we use an empty handler for events versus using storybook actions?

[broccolinisoup]

This is a really good question. I am not really sure either. I find storybook actions useful when the story is about the action like the data table action stories you added are great examples for this use case in my opinion. The stories I added or the ones I mentioned here to update are not focusing on actions, they are focusing on semantics, visuals, and having group titles. This is why, I didn't worry about the action itself but I am more than happy to update them to use storybook actions if it is going to be helpful.
Re-thinking about it now, it is probably helpful to log something (storybook actions could be very beneficial in this case) when the click event is trigger at least so empty event handler isn't very helpful here.

What are your thoughts??

[joshblack]

In this case, are GroupHeading and slots.groupHeading the same component? If so, would cloneElement work in this scenario?

Suggested change

	<GroupHeading
	title={title}
	variant={variant}
	auxiliaryText={auxiliaryText}
	groupHeadingId={groupHeadingId}
	as={slots.groupHeading?.props.as}
	>
	{slots.groupHeading ? slots.groupHeading.props.children : null}
	</GroupHeading>
	{cloneElement(slots.groupHeading, {
	title,
	variant,
	auxiliaryText,
	groupHeadingId,
	})}

[broccolinisoup]

They are the same components yes.

When I tried cloning though it is complaining about slots.groupHeading might be undefined which is a valid case. If ActionList is using the "to-be-deprecated" API which is <ActionList.Group title="Group heading"> , slots.groupHeading is going to be undefined because I use GroupHeading component to render both (title="Group heading" and <ActionList.GoupHeading>Group heading</GroupHeading>),

They are almost the same component except one is taking title prop and the other is taking children prop to render the heading. I can try splitting them up, or if you have any suggestion I would love to hear it!

[joshblack]

Looks great! Just left a comment with that idea from Slack, figured it'd be more helpful than the snippet I shared there 😂