Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrate Icon component into Bento and Card #798

Merged
merged 13 commits into from
Oct 31, 2024
Merged
5 changes: 5 additions & 0 deletions .changeset/little-bugs-greet.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/brand-primitives': minor
---

⚠️ Breaking change: Removed all Card-icon-background/color tokens (eg `--brand-Card-icon-background-blue`, `--brand-Card-icon-color-orange`)
6 changes: 6 additions & 0 deletions .changeset/many-rivers-drop.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@primer/react-brand': patch
---

- Fixed a bug where the `Bento` `leadingVisual` prop wouldn't honour the provided `size`.
- Updated the `Card.Icon` internal implementation to use the new `Icon` component.
16 changes: 8 additions & 8 deletions apps/docs/content/components/Card/react.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ You can add an `icon` to enhance the visual context. We recommend using an [Octi

```jsx live
<Card href="https://github.com">
<Card.Icon icon={<CopilotIcon />} color="purple" hasBackground />
<Card.Icon icon={CopilotIcon} color="purple" hasBackground />
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does Icon still support passing the instantiated component, or only reference? I.e. if user wanted to customize values here, can they still do it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that was removed by design after speaking with @danielguillan to avoid situations like this:

<Card.Icon icon={<CopilotIcon size={20} />} size={44} />

Since Card.Icon forwards all of its props to the provided icon, you could easily end up in these awkward situations where two props clash.

Do you think this should be called out as a breaking change in the changeset?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this example. Makes sense 👍

This is a breaking change if we don't allow instantiated JSX anymore, yeah. Unless there's a really important reason for it, we should continue to support both ways, as we have many instances in dotcom and elsewhere that use that approach and we can more easily work around it on our side than the users. In many of those instances, the user doesn't apply size too, so this is less of a risk IMO.

E.g.

Screenshot 2024-10-31 at 10 44 51

Suggestion:

Update our docs and examples to show the reference approach (happy path), and ensure that instantiated JSX can continue to be passed for continuity. How does that sound?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update for posterity.. I've pushed the changes ☝️ in this commit

  • passing JSX is now working
  • removed breaking change notice
  • added a new story to track future regressions

<Card.Heading>Code search & code view</Card.Heading>
<Card.Description>
Enables you to rapidly search, navigate, and understand code, right from
Expand Down Expand Up @@ -113,7 +113,7 @@ Combine `Icon` and `Label` when you need to add more metadata or context to the

```jsx live
<Card href="https://github.com">
<Card.Icon icon={<CopilotIcon />} color="green" hasBackground />
<Card.Icon icon={CopilotIcon} color="green" hasBackground />
<Card.Label color="blue-purple">Beta</Card.Label>
<Card.Heading>Code search & code view</Card.Heading>
<Card.Description>
Expand Down Expand Up @@ -153,7 +153,7 @@ The color of the lit area corrosponds to the nearest active, primary accent colo
<Box padding="spacious" backgroundColor="default">
<Stack direction="horizontal" padding="none">
<Card href="https://github.com">
<Card.Icon icon={<CopilotIcon />} color="pink" hasBackground />
<Card.Icon icon={CopilotIcon} color="pink" hasBackground />
<Card.Heading>Code search & code view</Card.Heading>
<Card.Description>
Enables you to rapidly search, navigate, and understand code, right
Expand All @@ -166,7 +166,7 @@ The color of the lit area corrosponds to the nearest active, primary accent colo
['--brand-color-accent-primary']: 'var(--base-color-scale-lime-2)',
}}
>
<Card.Icon icon={<ZapIcon />} color="lime" hasBackground />
<Card.Icon icon={ZapIcon} color="lime" hasBackground />
<Card.Heading>Code search & code view</Card.Heading>
<Card.Description>
Enables you to rapidly search, navigate, and understand code, right
Expand Down Expand Up @@ -209,21 +209,21 @@ Use the `Stack` component to stack multiple cards horizontally or vertically.
gap="normal"
>
<Card href="https://github.com">
<Card.Icon icon={<CopilotIcon />} color="indigo" hasBackground />
<Card.Icon icon={CopilotIcon} color="indigo" hasBackground />
<Card.Heading>Heading</Card.Heading>
<Card.Description>
Everything you need to know about getting started with GitHub Actions.
</Card.Description>
</Card>
<Card href="https://github.com">
<Card.Icon icon={<RocketIcon />} color="purple" hasBackground />
<Card.Icon icon={RocketIcon} color="purple" hasBackground />
<Card.Heading>Heading</Card.Heading>
<Card.Description>
Everything you need to know about getting started with GitHub Actions.
</Card.Description>
</Card>
<Card href="https://github.com">
<Card.Icon icon={<GitBranchIcon />} color="teal" hasBackground />
<Card.Icon icon={GitBranchIcon} color="teal" hasBackground />
<Card.Heading>Heading</Card.Heading>
<Card.Description>
Everything you need to know about getting started with GitHub Actions.
Expand All @@ -246,7 +246,7 @@ Use the `Stack` component to stack multiple cards horizontally or vertically.

### Card.Image

Forwards all the props from the [Image component](/components/Image), including `src`, `alt`, and `aspecRatio`.
Forwards all the props from the [Image component](/components/Image), including `src`, `alt`, and `aspectRatio`.

### Card.Icon

Expand Down
4 changes: 2 additions & 2 deletions apps/docs/content/getting-started/animation.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ Alternatively, use `delay` to stagger animations on adjacent elements if this fe
>
<Animate animate="scale-in-up">
<Card href="https://github.com">
<Card.Icon icon={<CopilotIcon />} color="indigo" hasBackground />
<Card.Icon icon={CopilotIcon} color="indigo" hasBackground />
<Card.Label>Limited</Card.Label>
<Card.Heading>
Collaboration is the key to DevOps success
Expand All @@ -198,7 +198,7 @@ Alternatively, use `delay` to stagger animations on adjacent elements if this fe
>
<Animate animate="scale-in-up">
<Card href="https://github.com">
<Card.Icon icon={<RocketIcon />} hasBackground color="blue" />
<Card.Icon icon={RocketIcon} hasBackground color="blue" />
<Card.Label>Limited</Card.Label>
<Card.Heading>GitHub Actions cheat sheet and more</Card.Heading>
<Card.Description>
Expand Down
12 changes: 6 additions & 6 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions packages/design-tokens/scripts/build-tokens.js
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,7 @@ const darkJson = require('../src/tokens/base/colors/dark')
`tokens/functional/components/tooltip/colors.json`,
`tokens/functional/components/river-story-scroll/colors.js`,
`tokens/functional/components/pricing-options/colors.json`,
`tokens/functional/components/icon/colors.json`,
]

for (const path of filesForColorModes) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,124 +5,6 @@
"value": "var(--brand-color-canvas-default)",
"dark": "var(--brand-color-canvas-subtle)"
}
},
"icon": {
joshfarrant marked this conversation as resolved.
Show resolved Hide resolved
"background": {
"default": {
"value": "var(--base-color-scale-gray-0)",
"dark": "var(--base-color-scale-gray-6)"
},
"blue": {
"value": "var(--base-color-scale-blue-0)",
"dark": "var(--base-color-scale-blue-8)"
},
"coral": {
"value": "var(--base-color-scale-coral-0)",
"dark": "var(--base-color-scale-coral-8)"
},
"green": {
"value": "var(--base-color-scale-green-0)",
"dark": "var(--base-color-scale-green-8)"
},
"gray": {
"value": "var(--base-color-scale-gray-0)",
"dark": "var(--base-color-scale-gray-6)"
},
"indigo": {
"value": "var(--base-color-scale-indigo-0)",
"dark": "var(--base-color-scale-indigo-8)"
},
"lemon": {
"value": "var(--base-color-scale-lemon-0)",
"dark": "var(--base-color-scale-lemon-8)"
},
"lime": {
"value": "var(--base-color-scale-lime-0)",
"dark": "var(--base-color-scale-lime-8)"
},
"orange": {
"value": "var(--base-color-scale-orange-0)",
"dark": "var(--base-color-scale-orange-8)"
},
"pink": {
"value": "var(--base-color-scale-pink-0)",
"dark": "var(--base-color-scale-pink-8)"
},
"purple": {
"value": "var(--base-color-scale-purple-0)",
"dark": "var(--base-color-scale-purple-8)"
},
"red": {
"value": "var(--base-color-scale-red-0)",
"dark": "var(--base-color-scale-red-8)"
},
"teal": {
"value": "var(--base-color-scale-teal-0)",
"dark": "var(--base-color-scale-teal-8)"
},
"yellow": {
"value": "var(--base-color-scale-yellow-0)",
"dark": "var(--base-color-scale-yellow-8)"
}
},
"color": {
"default": {
"value": "var(--brand-color-scale-gray-5)",
"dark": "var(--brand-color-scale-gray-2)"
},
"blue": {
"value": "var(--base-color-scale-blue-5)",
"dark": "var(--base-color-scale-blue-2)"
},
"coral": {
"value": "var(--base-color-scale-coral-5)",
"dark": "var(--base-color-scale-coral-2)"
},
"green": {
"value": "var(--base-color-scale-green-5)",
"dark": "var(--base-color-scale-green-2)"
},
"gray": {
"value": "var(--base-color-scale-gray-6)",
"dark": "var(--base-color-scale-gray-2)"
},
"indigo": {
"value": "var(--base-color-scale-indigo-5)",
"dark": "var(--base-color-scale-indigo-2)"
},
"lemon": {
"value": "var(--base-color-scale-lemon-5)",
"dark": "var(--base-color-scale-lemon-2)"
},
"lime": {
"value": "var(--base-color-scale-lime-5)",
"dark": "var(--base-color-scale-lime-2)"
},
"orange": {
"value": "var(--base-color-scale-orange-5)",
"dark": "var(--base-color-scale-orange-2)"
},
"pink": {
"value": "var(--base-color-scale-pink-5)",
"dark": "var(--base-color-scale-pink-2)"
},
"purple": {
"value": "var(--base-color-scale-purple-5)",
"dark": "var(--base-color-scale-purple-2)"
},
"red": {
"value": "var(--base-color-scale-red-5)",
"dark": "var(--base-color-scale-red-2)"
},
"teal": {
"value": "var(--base-color-scale-teal-5)",
"dark": "var(--base-color-scale-teal-2)"
},
"yellow": {
"value": "var(--base-color-scale-yellow-5)",
"dark": "var(--base-color-scale-yellow-2)"
}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@
},
"color": {
"default": {
"value": "var(--base-color-scale-gray-5)",
"dark": "var(--base-color-scale-gray-2)"
"value": "var(--brand-color-text-default)",
"dark": "var(--brand-color-text-default)"
},
"blue": {
"value": "var(--base-color-scale-blue-5)",
Expand Down
11 changes: 7 additions & 4 deletions packages/react/src/Bento/Bento.stories.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import React from 'react'
import {Meta} from '@storybook/react'
import {Bento} from '.'
import {Link} from '../Link'

import {CopilotIcon} from '@primer/octicons-react'
import {Bento} from '.'
import {Link} from '../'
import {Icon} from '../Icon'

export default {
title: 'Components/Bento',
Expand All @@ -22,7 +22,10 @@ export default {
export const Default = () => (
<Bento>
<Bento.Item rowSpan={{xsmall: 4, small: 5}} flow={{xsmall: 'row', small: 'row'}}>
<Bento.Content padding={{xsmall: 'normal', small: 'spacious'}} leadingVisual={<CopilotIcon />}>
<Bento.Content
padding={{xsmall: 'normal', small: 'spacious'}}
leadingVisual={<Icon icon={CopilotIcon} color="gray" />}
>
<Bento.Heading>
Push what&apos;s possible with GitHub Copilot, the world&apos;s most trusted and widely adopted AI developer
tool.
Expand Down
9 changes: 5 additions & 4 deletions packages/react/src/Bento/Bento.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
import React, {ReactHTML, ReactElement, forwardRef, useCallback, useMemo, Ref, PropsWithChildren} from 'react'
import clsx from 'clsx'
import {Icon, IconProps} from '@primer/octicons-react'
import {useWindowSize, BreakpointSize} from '../hooks/useWindowSize'
import type {BaseProps} from '../component-helpers'
import findElementInChildren from '../findElementInChildren'

import {Heading, Text, Link, HeadingProps, TextProps, LinkProps, ColorMode as FullColorMode, Image, Label} from '../'

import type {IconProps} from '../Icon'

import '@primer/brand-primitives/lib/design-tokens/css/tokens/functional/components/bento/base.css'
import styles from './Bento.module.css'

Expand Down Expand Up @@ -169,7 +170,7 @@ const Item = ({
}

type BentoContentProps = {
leadingVisual?: ReactElement | Icon
leadingVisual?: ReactElement<IconProps>
padding?: Padding | ResponsivePadding
verticalAlign?: Align | ResponsiveAlign
horizontalAlign?: Exclude<Align, 'end'> | Partial<Record<Size, Exclude<Align, 'end'>>>
Expand Down Expand Up @@ -203,9 +204,9 @@ const Content = ({
return (
<div className={clsx(styles[`Bento-padding--${padding}`], ...bentoContentClassArray, className)} {...rest}>
{React.isValidElement(LeadingVisual) &&
React.cloneElement(LeadingVisual as React.ReactElement<IconProps>, {
React.cloneElement(LeadingVisual as ReactElement<IconProps>, {
className: styles['Bento__Content-icon'],
size: LeadingVisual['size'] || 44,
size: LeadingVisual.props.size || 44,
})}

{React.isValidElement(LabelChild) &&
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Loading