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

Update Icons to use React.forwardRef & Remove support for Octicon #943

Merged
merged 8 commits into from
May 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/dry-buttons-leave.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@primer/octicons': major
---

Remove support for `Octicon`
Update peer dependency React version to support >=16.3
Update icons to use React.forwardRef
2 changes: 0 additions & 2 deletions lib/octicons_react/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -121,9 +121,7 @@ export default () => (
)
```

### `Octicon` (DEPRECATED)

> ⚠️ The `Octicon` component is deprecated. Use icon components on their own instead:
```diff
- <Octicon icon={AlertIcon} />
+ <AlertIcon />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,5 @@ exports[`@primer/octicons-react should not update exports without a semver chang
"ZapIcon",
"ZoomInIcon",
"ZoomOutIcon",
"default",
]
`;
2 changes: 1 addition & 1 deletion lib/octicons_react/__tests__/tree-shaking.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,5 +50,5 @@ test('tree shaking single export', async () => {
})

const bundleSize = Buffer.byteLength(output[0].code.trim()) / 1000
expect(`${bundleSize}kB`).toMatchInlineSnapshot(`"3.168kB"`)
expect(`${bundleSize}kB`).toMatchInlineSnapshot(`"3.244kB"`)
})
2 changes: 1 addition & 1 deletion lib/octicons_react/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@
"typescript": "^4.8.3"
},
"peerDependencies": {
"react": ">=15"
"react": ">=16.3"
},
"engines": {
"node": ">=8"
Expand Down
2 changes: 1 addition & 1 deletion lib/octicons_react/pages/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export default function App() {
<td>
<pre>
{`
import Octicon, {${Icon.name}} from '${pkg.name}'
import {${Icon.name}} from '${pkg.name}'
export default () => <${Icon.name} />
`.trim()}
</pre>
Expand Down
56 changes: 1 addition & 55 deletions lib/octicons_react/src/__tests__/octicon.js
Original file line number Diff line number Diff line change
@@ -1,61 +1,7 @@
import '@testing-library/jest-dom'
import {render} from '@testing-library/react'
import React from 'react'
import Octicon, {AlertIcon} from '../index'

describe('Octicon component', () => {
let mock

beforeEach(() => {
mock = jest.spyOn(console, 'warn').mockImplementation(() => jest.fn())
})

afterEach(() => {
expect(mock).toHaveBeenCalledWith(
expect.stringContaining(
`<Octicon> is deprecated. Use icon components on their own instead (e.g. <Octicon icon={AlertIcon} /> → <AlertIcon />)`
)
)
mock.mockRestore()
})

it('throws an error without a single child or icon prop', () => {
const spy = jest.spyOn(console, 'error').mockImplementation(() => jest.fn())
expect(() => render(<Octicon />)).toThrow()
expect(() => render(<Octicon icon={null} />)).toThrow()
expect(spy).toHaveBeenCalledWith(
expect.objectContaining({
message: expect.stringContaining('React.Children.only expected to receive a single React element child')
})
)

spy.mockRestore()
})

it('passes props to icon prop', () => {
const {container} = render(
<Octicon aria-label="icon" className="foo" size={20} verticalAlign="middle" icon={AlertIcon} />
)
expect(container.querySelector('svg')).toHaveAttribute('aria-label', 'icon')
expect(container.querySelector('svg')).toHaveAttribute('class', 'foo')
expect(container.querySelector('svg')).toHaveAttribute('width', '20')
expect(container.querySelector('svg')).toHaveAttribute('height', '20')
expect(container.querySelector('svg')).toHaveStyle({verticalAlign: 'middle'})
})

it('passes props to icon as child', () => {
const {container} = render(
<Octicon aria-label="icon" className="foo" size={20} verticalAlign="middle">
<AlertIcon />
</Octicon>
)
expect(container.querySelector('svg')).toHaveAttribute('aria-label', 'icon')
expect(container.querySelector('svg')).toHaveAttribute('class', 'foo')
expect(container.querySelector('svg')).toHaveAttribute('width', '20')
expect(container.querySelector('svg')).toHaveAttribute('height', '20')
expect(container.querySelector('svg')).toHaveStyle({verticalAlign: 'middle'})
})
})
import {AlertIcon} from '../index'

describe('An icon component', () => {
it('matches snapshot', () => {
Expand Down
78 changes: 42 additions & 36 deletions lib/octicons_react/src/createIconComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,43 +10,49 @@ export function createIconComponent(name, defaultClassName, getSVGData) {
const svgDataByHeight = getSVGData()
const heights = Object.keys(svgDataByHeight)

function Icon({
'aria-label': ariaLabel,
tabIndex,
className = defaultClassName,
fill = 'currentColor',
size = 16,
verticalAlign = 'text-bottom'
}) {
const height = sizeMap[size] || size
const naturalHeight = closestNaturalHeight(heights, height)
const naturalWidth = svgDataByHeight[naturalHeight].width
const width = height * (naturalWidth / naturalHeight)
const path = svgDataByHeight[naturalHeight].path
const Icon = React.forwardRef(
(
{
'aria-label': ariaLabel,
tabIndex,
className = defaultClassName,
fill = 'currentColor',
size = 16,
verticalAlign = 'text-bottom'
},
forwardedRef
) => {
const height = sizeMap[size] || size
const naturalHeight = closestNaturalHeight(heights, height)
const naturalWidth = svgDataByHeight[naturalHeight].width
const width = height * (naturalWidth / naturalHeight)
const path = svgDataByHeight[naturalHeight].path

return (
<svg
aria-hidden={ariaLabel ? 'false' : 'true'}
tabIndex={tabIndex}
focusable={tabIndex >= 0 ? 'true' : 'false'}
aria-label={ariaLabel}
role="img"
className={className}
viewBox={`0 0 ${naturalWidth} ${naturalHeight}`}
width={width}
height={height}
fill={fill}
style={{
display: 'inline-block',
userSelect: 'none',
verticalAlign,
overflow: 'visible'
}}
>
{path}
</svg>
)
}
return (
<svg
ref={forwardedRef}
aria-hidden={ariaLabel ? 'false' : 'true'}
tabIndex={tabIndex}
focusable={tabIndex >= 0 ? 'true' : 'false'}
aria-label={ariaLabel}
role="img"
className={className}
viewBox={`0 0 ${naturalWidth} ${naturalHeight}`}
width={width}
height={height}
fill={fill}
style={{
display: 'inline-block',
userSelect: 'none',
verticalAlign,
overflow: 'visible'
}}
>
{path}
</svg>
)
}
)

Icon.displayName = name

Expand Down
7 changes: 0 additions & 7 deletions lib/octicons_react/src/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,4 @@ export interface OcticonProps {
verticalAlign?: 'middle' | 'text-bottom' | 'text-top' | 'top' | 'unset'
}

/**
* @deprecated Use icon components on their own instead (e.g. `<Octicon icon={AlertIcon} />` → `<AlertIcon />`)
*/
declare const Octicon: React.FC<OcticonProps>

export default Octicon

export * from './__generated__/icons'
11 changes: 0 additions & 11 deletions lib/octicons_react/src/index.js
Original file line number Diff line number Diff line change
@@ -1,12 +1 @@
import React from 'react'

export default function Octicon({icon: Icon, children, ...props}) {
// eslint-disable-next-line no-console
console.warn(
// eslint-disable-next-line github/unescaped-html-literal
'<Octicon> is deprecated. Use icon components on their own instead (e.g. <Octicon icon={AlertIcon} /> → <AlertIcon />)'
)
return typeof Icon === 'function' ? <Icon {...props} /> : React.cloneElement(React.Children.only(children), props)
}

export * from './__generated__/icons'
15 changes: 3 additions & 12 deletions lib/octicons_react/ts-tests/index.tsx
Original file line number Diff line number Diff line change
@@ -1,20 +1,11 @@
import * as React from 'react'
import Octicon, {MarkGithubIcon, PlusIcon, RepoIcon} from '../src'
import {MarkGithubIcon, PlusIcon, RepoIcon} from '../src'

function TestOcticons() {
return (
<div>
<Octicon icon={RepoIcon} size="large" verticalAlign="middle" /> github/github
<Octicon icon={PlusIcon} aria-label="Add new item" /> New
<Octicon icon={MarkGithubIcon} size="large" aria-label="GitHub" />
<Octicon icon={RepoIcon} className="awesomeClassName" />
<Octicon>
<RepoIcon />
</Octicon>
<Octicon size="large">
<RepoIcon />
</Octicon>
<RepoIcon />
<MarkGithubIcon />
<PlusIcon />
<RepoIcon size="medium" className="test" aria-label="repo" verticalAlign="middle" />
</div>
)
Expand Down