ActionList/ActionMenu API: Add trailingVisual alias

[siddharthkp]

• mark trailingIcon and trailingText as deprecated
• prefer trailingVisual but support old api as fallback
• Update docs: add trailingVisual example

Use as text:

<ActionList
  items={[
    {text: 'New file', trailingVisual: '⌘N'},
    {text: 'Edit file', trailingText: '⌘E'}, // backward compatible
    {text: 'Copy link'},
    {text: 'Delete file', variant: 'danger'}
  ]}
/>

---

Freeform, use with icon:

<ActionList 
  items={[{
    leadingVisual: SearchIcon,
    text: 'repo:github/github',
    trailingVisual: () => (
      <>
        ⌘S<ArrowRightIcon />
      </>
    )
  },
  {
    leadingVisual: SearchIcon,
    text: 'repo:github/github',
    trailingText: '⌘S', // backward compatible
    trailingIcon: ArrowRightIcon // backward compatible
  }]}
/>

---

Question: Should we drop direct string support and only support a component type to keep it consistent with leadingVisual?

items={[
  {
    text: 'New file',
-   trailingText: '⌘N'
-   trailingVisual: '⌘N'
+   trailingVisual: () => <>⌘N</>
  }
]}

// types:
- trailingVisual?: React.FunctionComponent<IconProps> | string
+ trailingVisual?: React.FunctionComponent

[changeset-bot]

🦋 Changeset detected

Latest commit: 9a7936f

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	Patch

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	53.92 KB (0%)
dist/browser.umd.js	54.29 KB (0%)

[dusave]

mark trailingIcon and trailingVisual as deprecated

@siddharthkp I think you mean mark trailingIcon and trailingText as deprecated

[siddharthkp]

@dusave Oh right! Updated :)

[vdepizzol]

@siddharthkp I think this looks great! My only concern is when dealing with more complex scenarios where there's the need for both string + icon, like the example you gave:

(...)
trailingVisual: () => (
  <>
    ⌘S<ArrowRightIcon />
  </>
)
(...)

That could appear in a future multi-level overlay scenario (with the trailing text being used to describe the selected value, and clicking on that item drills-in to another view where the selection happens). Thinking about the rendering of these elements, there should be an 8px spacing between the string and ArrowRightIcon. How could we make this spacing come for free?

Question: Should we drop direct string support and only support a component type to keep it consistent with leadingVisual?

leadingVisual and trailingVisual differ a little bit because the trailing one does accept wider elements, such as inline text (which shouldn't really be used in leadingVisual). So I'd say it's ok to have some inconsistency.

✌️

[pksjce]

I was wondering what's the benefit of combining these two props to one? Is it to reduce api surface area?

[siddharthkp]

Thinking about the rendering of these elements, there should be an 8px spacing between the string and ArrowRightIcon. How could we make this spacing come for free?

We could add a space in the component but not sure if that's a reliable way to do it when it's a "bring your own content". 🤔

I was wondering what's the benefit of combining these two props to one? Is it to reduce api surface area?

Yep! Make it easy to guess. ← leadingVisual trailingVisual →

 

trailingText probably doesn't qualify as "visual" though

@vdepizzol Do you see trailingText as a separate piece outside trailingVisual? I don't love the idea of having an extra text key, but I could live with it until https://github.com/github/primer/issues/307 😅

[dusave]

Thinking about the rendering of these elements, there should be an 8px spacing between the string and ArrowRightIcon. How could we make this spacing come for free?

As a consumer, please no auto-spacing. I would prefer to control my content, add spacing as needed (as well as any other styling).

I was wondering what's the benefit of combining these two props to one? Is it to reduce api surface area?

As @siddharthkp said, this is to align with the align with the already-converted leadingVisual, which was changed in order to allow other elements as a leading element. It should be flexible enough to take a react element on either side.

[pksjce]

Ah I see, so if leadingVisual needed to have visual and text, we could just pass in a component to do that. I guess same could apply to trailingVisual. You could type it only as a react component and that could include text albeit slightly more markup?

[colebemis]

@siddharthkp Can you add a changeset for these changes?

[siddharthkp]

Ah I see, so if leadingVisual needed to have visual and text, we could just pass in a component to do that. I guess same could apply to trailingVisual. You could type it only as a react component and that could include text albeit slightly more markup?

leadingVisual is a bit more locked in. There's only space for either an Icon or Avatar. I merged the types for trailingText and trailingIcon for trailingVisual, but we could probably put React.ReactNode and be good - depending on how flexible we want to be with that space.

[siddharthkp]

Updated trailingVisual type to React.ReactNode. This PR is ready to go