ActionList.Item accepts a polymorphic 'as' prop #1463

jfuchs · 2021-09-24T17:51:26Z

This PR adds an as prop to ActionList.Item so that Primer users can put HTML Anchors, Next.js Links, React Router Links, or whatever else in their action lists.

I am adding a dependency (@radix-ui/react-polymorphic) for some type definitions. The library is:

MIT licensed
Pretty widely used
Has no dependences
Shouldn't add any bytes to bundles as it is just types

Closes #1413
Closes #1454

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 · 2021-09-24T17:51:28Z

🦋 Changeset detected

Latest commit: 7cfe964

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

This PR includes changesets to release 1 package

Name	Type
@primer/components	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

jfuchs · 2021-09-24T17:52:46Z

src/ActionList/List.tsx

    return (
      <ItemComponent
        showDivider={props.showItemDividers}
        selectionVariant={props.selectionVariant}
        {...itemProps}
        key={key}
        sx={{...itemStyle, ...itemProps.sx}}
-        item={item}


@dgreif after my changes, tsc was complaining about this line, and I honestly couldn't figure out what the intent was here. Any ideas? Is it safe to remove?

It's used by the callback handlers for onAction if I remember correctly. This comes back to the object equivalence issue for selection. We couldn't just pass an item "like" the one that was selected, we need the actual item that caused the ItemComponent to get rendered in the first place. That whole system has made a lot of things more complicated then I would like them to be 😅

Ohh that makes sense. I was wondering about that callback too. Any idea why the item in the callback was necessary?

(that is, I can't picture the usage where this is helping simplify things...why wouldn't the user just keep their item in the closure of the callback?)

I think the item in the callback was for the same reason. Higher components listening to the callback need the exact item to store which thing got selected

github-actions · 2021-09-24T17:54:54Z

size-limit report 📦

Path	Size
dist/browser.esm.js	55.41 KB (+0.03% 🔺)
dist/browser.umd.js	55.73 KB (+0.02% 🔺)

smockle · 2021-09-25T04:17:59Z

src/ActionList/List.tsx

@@ -162,7 +164,7 @@ export const List = React.forwardRef<HTMLDivElement, ListProps>((props, forwarde
  const renderItem = (itemProps: ItemInput, item: ItemInput, itemIndex: number) => {
    // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
    const ItemComponent = ('renderItem' in itemProps && itemProps.renderItem) || props.renderItem || Item
-    const key = itemProps.key ?? itemProps.id?.toString() ?? itemIndex.toString()
+    const key = itemProps.id?.toString() ?? itemIndex.toString()


Non-blocking: Out of curiosity, why was this change to key necessary?

Good catch — this was my mistake. TS caught that key wasn't on ItemInput so I removed the line intending to circle back to it. Will fix!

smockle · 2021-09-25T04:33:39Z

src/stories/ActionList.stories.tsx

+    // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+    // @ts-ignore
+    return React.cloneElement(child, childProps)


Does this accomplish the same result without a @ts-ignore?

Suggested change

// eslint-disable-next-line @typescript-eslint/ban-ts-comment

// @ts-ignore

return React.cloneElement(child, childProps)

return (

<>

{React.isValidElement(child)

? React.cloneElement(child, childProps)

: null}

</>

);

colebemis · 2021-09-29T16:06:36Z

docs/content/ActionList.mdx

-| showItemDividers | `boolean`                           |      `false`      | Optional. If `true` dividers will be displayed above each `ActionList.Item` which does not follow an `ActionList.Header` or `ActionList.Divider`        |
+| Name             | Type                                |               Default               | Description                                                                                                                                             |
+| :--------------- | :---------------------------------- | :---------------------------------: | :------------------------------------------------------------------------------------------------------------------------------------------------------ | ------------------------------------------------------------------------------------- |
+| items            | `Array<ItemProps                    | (props: ItemProps) => JSX.Element>` | `undefined`                                                                                                                                             | Required. A list of item objects conforming to the `ActionList.Item` props interface. |


Looks like the | in the type is confusing Markdown:

colebemis · 2021-09-29T16:07:58Z

src/ActionList/Item.tsx

+  /**
+   * An item to pass back in the `onAction` callback, meant as
+   */
+  item?: ItemInput


Can you explain why we needed to add these props?

These three fields were because tsc told me to :-) ... but also, they all make sense and I wonder if they only worked before because of gaps in the type coverage

colebemis · 2021-09-29T16:09:57Z

src/ActionList/Item.tsx

-}
+}) as PolymorphicForwardRefComponent<'div', ItemProps>
+
+export {Item}


Is there a reason you wrote the export like this instead of doing: export const Item ... on line 318

Nope, will fix.

colebemis · 2021-09-29T16:11:56Z

src/ActionList/List.tsx

-export type ItemInput = ItemProps | (Partial<ItemProps> & {renderItem: typeof Item})
+type RenderItemFn = (props: ItemProps) => React.ReactElement
+
+export type ItemInput = ItemProps | ((Partial<ItemProps> & {renderItem: RenderItemFn}) & {key?: Key})


Just curious, what's the reason for including key here?

Same thing as the new props for item — added because tsc caught issues that it wasn't catching before. in this case, it's something we allow in an ItemInput but remove before the ItemInput becomes ItemProps.

docs/content/ActionList.mdx

colebemis · 2021-09-29T16:21:09Z

src/stories/ActionList.stories.tsx

+        <ActionList
+          items={[
+            {
+              text: 'A. Vanilla action',
+              renderItem: props => <ActionList.Item onAction={() => alert('hi?')} {...props} />
+            },
+            {
+              text: 'B. Vanilla link',
+              renderItem: props => <ActionList.Item as="a" href="/about" {...props} />
+            },
+            {
+              text: 'C. React Router link',
+              renderItem: props => <ActionList.Item as={ReactRouterLikeLink} to="/about" {...props} />
+            },
+            {
+              text: 'D. NextJS style',
+              renderItem: props => (
+                <NextJSLikeLink href="/about">
+                  <ActionList.Item as="a" {...props} />
+                </NextJSLikeLink>
+              )
+            }
+          ]}
+        />


Could we add a Jest test that will fail CI if ActionList.Item doesn't render the element passed through the as prop correctly?

I want to make sure we don't accidentally break this if we refactor ActionList.Item (which will likely happen soon)

Just added a behavesAsComponent test

Co-authored-by: Cole Bemis <colebemis@github.com>

colebemis

Looks great. Excellent work, @jfuchs ✨

docs/content/ActionList.mdx

src/ActionList/Item.tsx

Co-authored-by: Cole Bemis <colebemis@github.com>

jfuchs commented Sep 24, 2021

View reviewed changes

colebemis self-requested a review September 24, 2021 18:51

jfuchs force-pushed the jfuchs/list-items-as branch from 3cb4eeb to e0410f1 Compare September 24, 2021 20:58

github-actions bot deployed to Preview September 24, 2021 20:59 View deployment

jfuchs force-pushed the jfuchs/list-items-as branch from e0410f1 to d058ce6 Compare September 24, 2021 22:46

github-actions bot deployed to Preview September 24, 2021 22:47 View deployment

jfuchs marked this pull request as ready for review September 24, 2021 23:00

jfuchs requested a review from a team September 24, 2021 23:00

github-actions bot deployed to Preview September 24, 2021 23:01 View deployment

smockle reviewed Sep 25, 2021

View reviewed changes

jfuchs force-pushed the jfuchs/list-items-as branch from cc08895 to c629181 Compare September 27, 2021 17:05

github-actions bot deployed to Preview September 27, 2021 17:05 View deployment

github-actions bot deployed to Preview September 27, 2021 17:14 View deployment

github-actions bot deployed to Preview September 28, 2021 16:25 View deployment

github-actions bot deployed to Preview September 28, 2021 19:50 View deployment

github-actions bot deployed to Preview September 28, 2021 23:53 View deployment

colebemis reviewed Sep 29, 2021

View reviewed changes

github-actions bot deployed to Preview September 29, 2021 20:54 View deployment

github-actions bot deployed to Preview September 29, 2021 20:56 View deployment

jfuchs and others added 5 commits September 29, 2021 14:00

ActionList.Item accepts a polymorphic 'as' prop

15e3129

Add back key

0614f42

fixes

b1abc4d

Update docs/content/ActionList.mdx

2d97288

Co-authored-by: Cole Bemis <colebemis@github.com>

Update docs/content/ActionList.mdx

19c3010

Co-authored-by: Cole Bemis <colebemis@github.com>

jfuchs force-pushed the jfuchs/list-items-as branch from bd41b2e to 19c3010 Compare September 29, 2021 21:01

github-actions bot deployed to Preview September 29, 2021 21:01 View deployment

colebemis approved these changes Sep 29, 2021

View reviewed changes

docs/content/ActionList.mdx Outdated Show resolved Hide resolved

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

Apply suggestions from code review

c55c126

Co-authored-by: Cole Bemis <colebemis@github.com>

github-actions bot deployed to Preview September 30, 2021 15:36 View deployment

Merge branch 'main' into jfuchs/list-items-as

7cfe964

github-actions bot deployed to Preview September 30, 2021 15:44 View deployment

jfuchs merged commit bde3a03 into main Sep 30, 2021

jfuchs deleted the jfuchs/list-items-as branch September 30, 2021 17:13

primer-css mentioned this pull request Oct 1, 2021

Release Tracking #1481

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ActionList.Item accepts a polymorphic 'as' prop #1463

ActionList.Item accepts a polymorphic 'as' prop #1463

jfuchs commented Sep 24, 2021 •

edited

Loading

changeset-bot bot commented Sep 24, 2021 •

edited

Loading

jfuchs Sep 24, 2021

dgreif Sep 24, 2021

jfuchs Sep 24, 2021

jfuchs Sep 24, 2021

dgreif Sep 27, 2021

github-actions bot commented Sep 24, 2021 •

edited

Loading

smockle Sep 25, 2021

jfuchs Sep 27, 2021

smockle Sep 25, 2021

colebemis Sep 29, 2021

colebemis Sep 29, 2021

jfuchs Sep 29, 2021 •

edited

Loading

colebemis Sep 29, 2021

jfuchs Sep 29, 2021

colebemis Sep 29, 2021

jfuchs Sep 29, 2021

colebemis Sep 29, 2021

jfuchs Sep 29, 2021

colebemis left a comment

ActionList.Item accepts a polymorphic 'as' prop #1463

ActionList.Item accepts a polymorphic 'as' prop #1463

Conversation

jfuchs commented Sep 24, 2021 • edited Loading

Merge checklist

changeset-bot bot commented Sep 24, 2021 • edited Loading

🦋 Changeset detected

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

github-actions bot commented Sep 24, 2021 • edited Loading

size-limit report 📦

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

jfuchs Sep 29, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colebemis left a comment

Choose a reason for hiding this comment

jfuchs commented Sep 24, 2021 •

edited

Loading

changeset-bot bot commented Sep 24, 2021 •

edited

Loading

github-actions bot commented Sep 24, 2021 •

edited

Loading

jfuchs Sep 29, 2021 •

edited

Loading