ActionList.GroupHeading roll out improvements #4395

broccolinisoup · 2024-03-15T00:21:13Z

Changelog

New

Adds a @deprecated TS notice to the title prop on ActionList.Group sub component.
Adds an invariant check for cases where the action list is rendered as menu or listbox but as prop is provided on the ActionList.GroupHeading component. This is not needed and in fact it could be misleading. Because action lists whose roles are menu or listbox, will render their group headings are divs, not actual headings. (Reefer to the api docs for this accessibility info for now.)

<ActionList role= "menu">
  <ActionList.Group>
    <ActionList.GroupHeading as="h2"><ActionList.GroupHeading> //👈 `as` is not needed here and it is misleading. 
  </ActionList.Group>
</ActionList>

Changed

Update the warning to become error (invariant) for missing as prop on <ActionList.GroupHeading> for list role action lists.

<ActionList>
  <ActionList.Group>
    <ActionList.GroupHeading as="h2"><ActionList.GroupHeading> //  👈 `as` is necessary here to explicitly set the heading level
  </ActionList.Group>
</ActionList>

Update the title prop on ActionList.GroupHeading to _internalBackwardCompatibleTitle, because it is used for to distinguish the old api vs new api for backwards compatibility and we don't want consumers to mistake and use that instead of children. https://github.com/primer/react/pull/4395/files#r1533123778

Removed

Rollout strategy

Patch release
Minor release
Major release; if selected, include a written rollout or migration plan
None; if selected, include a brief description as to why

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
(GitHub staff only) Integration tests pass at github/github (Learn more about how to run integration tests)

changeset-bot · 2024-03-15T00:21:16Z

🦋 Changeset detected

Latest commit: e6a19bc

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 · 2024-03-15T00:24:40Z

size-limit report 📦

Path	Size
packages/react/dist/browser.esm.js	88.68 KB (+0.01% 🔺)
packages/react/dist/browser.umd.js	88.98 KB (+0.05% 🔺)

broccolinisoup · 2024-03-21T01:22:00Z

packages/react/src/ActionList/Group.tsx

@@ -85,8 +85,10 @@ export const Group: React.FC<React.PropsWithChildren<ActionListGroupProps>> = ({
    >
      <GroupContext.Provider value={{selectionVariant, groupHeadingId}}>
        {title && !slots.groupHeading ? (
-          <GroupHeading title={title} variant={variant} auxiliaryText={auxiliaryText} />
+          // Escape hatch: supports old API <ActionList.Group title="group title"> in a non breaking way
+          <GroupHeading variant={variant} auxiliaryText={auxiliaryText} unsafeTitle={title} />


To make sure that the invariant check doesn't go through on the old API so that it doesn't break any usecases in downstream repos, I kept the ActionList.GroupHeading sub component's API flexible. Especially, to distinguish where the group heading is coming from i.e. old api vs new api.

If it comes from old api (i.e. if it has title prop), I render the heading like

<GroupHeading variant={variant} auxiliaryText={auxiliaryText} title={title} />

if it uses the new api (if children exists),

<GroupHeading variant={variant} auxiliaryText={auxiliaryText} >{title}</GroupHeading>

Optional name idea, instead of unsafeTitle: internalBackwardCompatibleTitle

broccolinisoup · 2024-03-21T02:01:25Z

packages/react/src/ActionList/ActionList.test.tsx

@@ -262,8 +262,8 @@ describe('ActionList', () => {
    expect(heading).toBeInTheDocument()
    expect(heading).toHaveTextContent('Group Heading')
  })
-  it('should throw a warning if ActionList.Group is used without as prop when no role is specified (for list role)', async () => {
-    const spy = jest.spyOn(console, 'warn').mockImplementationOnce(() => {})
+  it('should throw an error if ActionList.GroupHeading is used without an `as` prop when no role is specified (for list role)', async () => {


This test fails https://github.com/primer/react/actions/runs/8368326991/job/22912262706?pr=4395 and I am so confused why - it is throwing an error and I am expecting the error in the test 🤷🏻‍♀️ Am I missing something obvious?

I think you have to explicitly expect it to throw instead of spy

example in primer: https://github.com/primer/react/blob/main/packages/react/src/__tests__/Autocomplete.test.tsx#L444
stack overflow: https://stackoverflow.com/a/46155381

Ahh, right! it did the trick, thanks so much 🙌🏻

siddharthkp · 2024-03-25T15:33:23Z

packages/react/src/ActionList/ActionList.features.stories.tsx

@@ -73,7 +74,8 @@ export const WithVisualListHeading = () => (
      </ActionList.Item>
    </ActionList.Group>

-    <ActionList.Group title="Advanced">
+    <ActionList.Group>
+      <ActionList.GroupHeading as="h4">Advanced</ActionList.GroupHeading>


Just checking, do we have a story that still uses the old API? Just to keep it around for regressions

Yes! I added them as dev stories https://github.com/primer/react/blob/main/packages/react/src/ActionList/ActionList.dev.stories.tsx and playwright snapshots them too to catch any regression 😈

siddharthkp

Looking good! 🎉

Waiting for the tests first before approving!

siddharthkp

Looking great! Ship it!

ActionList.GroupHeading roll out improvements

cbcd7b5

broccolinisoup temporarily deployed to github-pages March 15, 2024 00:24 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4395 March 15, 2024 00:24 Inactive

fix tests

86823a3

broccolinisoup temporarily deployed to github-pages March 15, 2024 02:04 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4395 March 15, 2024 02:05 Inactive

add a deprecated TS notice

f94e8d2

broccolinisoup mentioned this pull request Mar 18, 2024

deprecate title prop and throw warning #3984

Closed

12 tasks

broccolinisoup temporarily deployed to github-pages March 18, 2024 01:54 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4395 March 18, 2024 01:54 Inactive

broccolinisoup commented Mar 21, 2024

View reviewed changes

Create chilly-buckets-kick.md

9b21aa0

broccolinisoup marked this pull request as ready for review March 21, 2024 01:33

broccolinisoup requested a review from a team as a code owner March 21, 2024 01:33

broccolinisoup requested review from camertron and siddharthkp March 21, 2024 01:33

broccolinisoup temporarily deployed to github-pages March 21, 2024 01:36 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4395 March 21, 2024 01:37 Inactive

broccolinisoup commented Mar 21, 2024

View reviewed changes

siddharthkp reviewed Mar 25, 2024

View reviewed changes

broccolinisoup added 2 commits March 27, 2024 10:18

Merge branch 'main' into actionlist-group-heading-inprovements

5dc9a53

messed up the conflict

8eabc6e

broccolinisoup temporarily deployed to github-pages March 27, 2024 00:23 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4395 March 27, 2024 00:23 Inactive

code review comment address

4f154cf

broccolinisoup temporarily deployed to github-pages March 27, 2024 00:52 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4395 March 27, 2024 00:52 Inactive

fix test and add another case

114cac6

broccolinisoup requested a review from siddharthkp March 27, 2024 02:29

broccolinisoup temporarily deployed to github-pages March 27, 2024 02:31 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4395 March 27, 2024 02:32 Inactive

add an underscore to the internal bc title

e6a19bc

broccolinisoup deployed to github-pages March 27, 2024 02:35 — with GitHub Actions View deployment

github-actions bot deployed to storybook-preview-4395 March 27, 2024 02:36 View deployment

siddharthkp approved these changes Mar 27, 2024

View reviewed changes

broccolinisoup added this pull request to the merge queue Mar 27, 2024

Merged via the queue into main with commit c2557d1 Mar 27, 2024
31 checks passed

broccolinisoup deleted the actionlist-group-heading-inprovements branch March 27, 2024 23:09

primer bot mentioned this pull request Mar 27, 2024

Release tracking #4410

Merged

broccolinisoup mentioned this pull request Apr 30, 2024

deprecate title prop on ActionList.Group component on docs #4544

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ActionList.GroupHeading roll out improvements #4395

ActionList.GroupHeading roll out improvements #4395

broccolinisoup commented Mar 15, 2024 •

edited

Loading

changeset-bot bot commented Mar 15, 2024 •

edited

Loading

github-actions bot commented Mar 15, 2024 •

edited

Loading

broccolinisoup Mar 21, 2024

siddharthkp Mar 25, 2024 •

edited

Loading

broccolinisoup Mar 21, 2024 •

edited

Loading

siddharthkp Mar 25, 2024

broccolinisoup Mar 27, 2024

siddharthkp Mar 25, 2024

broccolinisoup Mar 27, 2024

siddharthkp left a comment •

edited

Loading

siddharthkp left a comment

ActionList.GroupHeading roll out improvements #4395

ActionList.GroupHeading roll out improvements #4395

Conversation

broccolinisoup commented Mar 15, 2024 • edited Loading

Changelog

New

Changed

Removed

Rollout strategy

Testing & Reviewing

Merge checklist

changeset-bot bot commented Mar 15, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Mar 15, 2024 • edited Loading

size-limit report 📦

broccolinisoup Mar 21, 2024

Choose a reason for hiding this comment

siddharthkp Mar 25, 2024 • edited Loading

Choose a reason for hiding this comment

broccolinisoup Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

siddharthkp Mar 25, 2024

Choose a reason for hiding this comment

broccolinisoup Mar 27, 2024

Choose a reason for hiding this comment

siddharthkp Mar 25, 2024

Choose a reason for hiding this comment

broccolinisoup Mar 27, 2024

Choose a reason for hiding this comment

siddharthkp left a comment • edited Loading

Choose a reason for hiding this comment

siddharthkp left a comment

Choose a reason for hiding this comment

broccolinisoup commented Mar 15, 2024 •

edited

Loading

changeset-bot bot commented Mar 15, 2024 •

edited

Loading

github-actions bot commented Mar 15, 2024 •

edited

Loading

siddharthkp Mar 25, 2024 •

edited

Loading

broccolinisoup Mar 21, 2024 •

edited

Loading

siddharthkp left a comment •

edited

Loading