[Modification Request] Update the icon in Bento.Item to appear the same as Card (with a background)

[rfearing]

Summary

Currently the Bento's LeadingVisual is an Octicon with no background.

There are now new design directions about how icons should render in Bento boxes, the same way they appear in cards. (c.c. @nsolerieu):

Corresponding issue: https://github.com/github/marketing-platform-services/issues/3553

Requested update:

There be an option for the circular icon as a child of Bento.Item

[joshfarrant]

Hey @rfearing 👋

Following on from the discussions in yesterday's Primer Brand Office Hours, @danielguillan and I think the best way to proceed is with a dedicated Icon component.

Adding a new component to the library typically comprises of a few steps:

1. API exploration
2. Figma implementation
3. React implementation

If you're still happy to work on the implementation of this yourself — which we'd be very grateful for — then here's some information on each of those steps.

API exploration

The purpose of an API Exploration is to flesh out the API of new components with the Primer Brand maintainers. This is where we can discuss component naming, what props the component will accept (and the types of those props), and highlight anything that is out-of-scope for this component.

As a starting point, you might want to take a look at the Card.Icon component as that already implements some props (icon, color, hasBackground) that we'll likely want to support in this new component.

Typically these API explorations take place in a FigJam, however we can do it any way you prefer. If you'd like, you can instead share your proposed types for the props that the component will accept as a comment in this issue. I've got some initial thoughts on this, so let me know if you'd like some support with this 🙂

Figma implementation

After speaking with @danielguillan, I believe there is already a private component in Figma which is used in the Card component. This will need to be made into a public-facing component so it can be consumed alongside other Primer Brand components in Figma.

@danielguillan is the best person to speak to for support on this.

React implementation

Once we've got the API of the component firmed-up, and the component is in Figma, then the component is ready to be implemented. By this point most of the decisions relating to this component will have been made will have been made, so building the component shouldn't be too big of a job. We'll also need to ensure that the component is tested, and that it has accompanying documentation.

I'd be happy to support with this.

Next steps

Thanks again for offering to help with adding this component to the library. The process above might sound like a lot when it's all written out like this, but in practice it shouldn't be too big of a job. Of course, we're here to help with all stages too.

If you're happy to kick off the API exploration in this issue then we can get the ball rolling! ⚽ 🚀

[rfearing]

@joshfarrant Thank you so much for the super awesome explanation! Let's do it!

I really like the API of the Card.Icon. I believe they could be identical (or nearly identical):

type IconProps = BaseProps<HTMLSpanElement> & {
  icon: Icon // from @primer/octicons-react
  color?: (typeof IconColors)[number] // same as CardIconColors?
  hasBackground?: boolean
  ['aria-hidden']?: boolean // For Bento this would be true
}

(Note, I'm happy to start a figjam if that's what's best for Brand worfklow)

[joshfarrant]

That looks good to me @rfearing! 🚀

My only suggestions would be to maybe have it extend HTMLAttributes<HTMLSpanElement> (or HTMLDivElement if you think a div is more appropriate). If we do that then we can remove the explicit aria-hidden in the type as it will be covered by HTMLAttributes.

We also don't really need the BaseProps as this component doesn't support the animate prop, which BaseProps would suggest it does. There's a wider conversation to be had around the utility of BaseProps as a whole I think, so feel free to drop it / keep it as you like.

I'd be keen to get @danielguillan's thoughts on the supported colours, but keeping it consistent with CardIconColors makes sense to me 👍

type IconProps = {
  icon: Icon
  color?: (typeof IconColors)[number]
  hasBackground?: boolean
} & HTMLAttributes<HTMLSpanElement>

The only other thing to think about is the default values of color and hasBackground. Keeping those the same as Card.Icon works for me too 🙂

[rfearing]

@joshfarrant I just realized the Primer Icon has a size prop. Do you think we should include that? Card nor bento have it, but I could see it being useful.

[joshfarrant]

Good spot @rfearing. For now, I'd lean towards not adding it. We can bring that in as an enhancement in the future, but to keep things consistent my leaning would be to not add it for now