Skip to content

Commit 628e601

Browse files
joshblackfrancinelucca
joshblack
and
francinelucca
authored
feat(Heading): remove support for sx prop (#6860)
Co-authored-by: Marie Lucca <[email protected]> Co-authored-by: Marie Lucca <[email protected]>
1 parent 158703b commit 628e601

File tree

8 files changed

+32
-164
lines changed

8 files changed

+32
-164
lines changed

‎.changeset/weak-papers-drive.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@primer/react': major
3+
---
4+
5+
Remove support for `sx` from the `Heading` component

‎packages/react/src/Header/Header.dev.module.css‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,3 +11,7 @@
1111
.HeaderDevLink {
1212
color: var(--color-prettylights-syntax-carriage-return-text);
1313
}
14+
15+
.Icon {
16+
margin-right: var(--base-size-8);
17+
}

‎packages/react/src/Heading/Heading.docs.json‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,6 @@
66
"stories": [],
77
"importPath": "@primer/react",
88
"props": [
9-
{
10-
"name": "sx",
11-
"type": "SystemStyleObject",
12-
"deprecated": true
13-
},
149
{
1510
"name": "as",
1611
"type": "React.ElementType",

‎packages/react/src/Heading/Heading.features.stories.tsx‎

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -5,17 +5,6 @@ export default {
55
title: 'Components/Heading/Features',
66
}
77

8-
export const TestSx: StoryFn<typeof Heading> = () => (
9-
<Heading
10-
sx={{
11-
fontSize: 2,
12-
fontWeight: 'normal',
13-
}}
14-
>
15-
Heading with sx override
16-
</Heading>
17-
)
18-
198
export const Small: StoryFn<typeof Heading> = () => <Heading variant="small">Small heading</Heading>
209

2110
export const Medium: StoryFn<typeof Heading> = () => <Heading variant="medium">Medium heading</Heading>

‎packages/react/src/Heading/Heading.tsx‎

Lines changed: 5 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,16 @@
11
import {clsx} from 'clsx'
22
import React, {forwardRef, useEffect} from 'react'
33
import {useRefObjectAsForwardedRef} from '../hooks'
4-
import type {SxProp} from '../sx'
54
import type {ComponentProps} from '../utils/types'
65
import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic'
76
import classes from './Heading.module.css'
8-
import Box from '../Box'
7+
8+
type HeadingLevels = 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6'
99

1010
type StyledHeadingProps = {
11-
as?: 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6'
11+
as?: HeadingLevels
1212
variant?: 'large' | 'medium' | 'small'
13-
} & SxProp
13+
}
1414

1515
const Heading = forwardRef(({as: Component = 'h2', className, variant, ...props}, forwardedRef) => {
1616
const innerRef = React.useRef<HTMLHeadingElement>(null)
@@ -33,20 +33,8 @@ const Heading = forwardRef(({as: Component = 'h2', className, variant, ...props}
3333
}, [innerRef])
3434
}
3535

36-
if (props.sx) {
37-
return (
38-
<Box
39-
as={Component}
40-
className={clsx(className, classes.Heading)}
41-
data-variant={variant}
42-
{...props}
43-
// @ts-ignore temporary disable as we migrate to css modules, until we remove PolymorphicForwardRefComponent
44-
ref={innerRef}
45-
/>
46-
)
47-
}
4836
return <Component className={clsx(className, classes.Heading)} data-variant={variant} {...props} ref={innerRef} />
49-
}) as PolymorphicForwardRefComponent<'h2', StyledHeadingProps>
37+
}) as PolymorphicForwardRefComponent<HeadingLevels, StyledHeadingProps>
5038

5139
Heading.displayName = 'Heading'
5240

‎packages/react/src/Heading/__tests__/Heading.test.tsx‎

Lines changed: 0 additions & 130 deletions
Original file line numberDiff line numberDiff line change
@@ -1,30 +1,6 @@
11
import {describe, expect, it, vi} from 'vitest'
22
import {Heading} from '../..'
33
import {render, screen} from '@testing-library/react'
4-
import ThemeProvider from '../../ThemeProvider'
5-
6-
const theme = {
7-
breakpoints: ['400px', '640px', '960px', '1280px'],
8-
colors: {
9-
green: ['#010', '#020', '#030', '#040', '#050', '#060'],
10-
},
11-
fontSizes: ['12px', '14px', '16px', '20px', '24px', '32px', '40px', '48px'],
12-
fonts: {
13-
normal: 'Helvetica,sans-serif',
14-
mono: 'Consolas,monospace',
15-
},
16-
lineHeights: {
17-
normal: 1.5,
18-
condensed: 1.25,
19-
condensedUltra: 1,
20-
},
21-
fontWeights: {
22-
light: '300',
23-
normal: '400',
24-
semibold: '500',
25-
bold: '600',
26-
},
27-
}
284

295
describe('Heading', () => {
306
it('should support `className` on the outermost element', () => {
@@ -38,89 +14,6 @@ describe('Heading', () => {
3814
expect(heading.tagName).toBe('H2')
3915
})
4016

41-
it('respects fontWeight', () => {
42-
const {container} = render(
43-
<ThemeProvider theme={theme}>
44-
<Heading sx={{fontWeight: 'bold'}} />
45-
</ThemeProvider>,
46-
)
47-
const heading = container.firstChild as HTMLElement
48-
expect(heading).toHaveStyle(`font-weight: ${theme.fontWeights.bold}`)
49-
50-
const {container: container2} = render(
51-
<ThemeProvider theme={theme}>
52-
<Heading sx={{fontWeight: 'normal'}} />
53-
</ThemeProvider>,
54-
)
55-
const heading2 = container2.firstChild as HTMLElement
56-
expect(heading2).toHaveStyle(`font-weight: ${theme.fontWeights.normal}`)
57-
58-
const {container: container3} = render(
59-
<ThemeProvider theme={theme}>
60-
<Heading sx={{fontWeight: 'semibold'}} />
61-
</ThemeProvider>,
62-
)
63-
const heading3 = container3.firstChild as HTMLElement
64-
expect(heading3).toHaveStyle(`font-weight: ${theme.fontWeights.semibold}`)
65-
66-
const {container: container4} = render(
67-
<ThemeProvider theme={theme}>
68-
<Heading sx={{fontWeight: 'light'}} />
69-
</ThemeProvider>,
70-
)
71-
const heading4 = container4.firstChild as HTMLElement
72-
expect(heading4).toHaveStyle(`font-weight: ${theme.fontWeights.light}`)
73-
})
74-
75-
it('respects lineHeight', () => {
76-
const {container} = render(
77-
<ThemeProvider theme={theme}>
78-
<Heading sx={{lineHeight: 'normal'}} />
79-
</ThemeProvider>,
80-
)
81-
const heading = container.firstChild as HTMLElement
82-
///These sx tests should go away right?
83-
expect(heading).toHaveStyle(`line-height: 48px`)
84-
85-
const {container: container2} = render(
86-
<ThemeProvider theme={theme}>
87-
<Heading sx={{lineHeight: 'condensed'}} />
88-
</ThemeProvider>,
89-
)
90-
const heading2 = container2.firstChild as HTMLElement
91-
expect(heading2).toHaveStyle(`line-height: 40px`)
92-
93-
const {container: container3} = render(
94-
<ThemeProvider theme={theme}>
95-
<Heading sx={{lineHeight: 'condensedUltra'}} />
96-
</ThemeProvider>,
97-
)
98-
const heading3 = container3.firstChild as HTMLElement
99-
expect(heading3).toHaveStyle(`line-height: 32px`)
100-
})
101-
102-
it('respects fontFamily="mono"', () => {
103-
const {container} = render(
104-
<ThemeProvider theme={theme}>
105-
<Heading sx={{fontFamily: 'mono'}} />
106-
</ThemeProvider>,
107-
)
108-
const heading = container.firstChild as HTMLElement
109-
expect(heading).toHaveStyle(`font-family: ${theme.fonts.mono}`)
110-
})
111-
112-
it('renders fontSize', () => {
113-
for (const fontSize of theme.fontSizes) {
114-
const {container} = render(
115-
<ThemeProvider theme={theme}>
116-
<Heading sx={{fontSize}} />
117-
</ThemeProvider>,
118-
)
119-
const heading = container.firstChild as HTMLElement
120-
expect(heading).toHaveStyle(`font-size: ${fontSize}`)
121-
}
122-
})
123-
12417
it('logs a warning when trying to render invalid "as" prop', () => {
12518
const consoleSpy = vi.spyOn(globalThis.console, 'warn').mockImplementation(() => {})
12619

@@ -130,15 +23,6 @@ describe('Heading', () => {
13023

13124
consoleSpy.mockRestore()
13225
})
133-
it('respects the "fontStyle" prop', () => {
134-
const {container} = render(
135-
<ThemeProvider theme={theme}>
136-
<Heading sx={{fontStyle: 'italic'}} />
137-
</ThemeProvider>,
138-
)
139-
const heading = container.firstChild as HTMLElement
140-
expect(heading).toHaveStyle('font-style: italic')
141-
})
14226

14327
// How can we test for generated class names?
14428
it.skip('should only include css modules class', () => {
@@ -148,18 +32,4 @@ describe('Heading', () => {
14832
// for this component
14933
expect(screen.getByText('test')).not.toHaveClass(/^Heading__StyledHeading/)
15034
})
151-
152-
it('should support overrides with sx if provided', () => {
153-
render(
154-
<Heading
155-
sx={{
156-
fontWeight: '900',
157-
}}
158-
>
159-
test
160-
</Heading>,
161-
)
162-
163-
expect(screen.getByText('test')).toHaveStyle('font-weight: 900')
164-
})
16535
})

‎packages/styled-react/src/components/Heading.tsx‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
import {Heading as PrimerHeading} from '@primer/react'
2+
import {type HeadingProps as PrimerHeadingProps} from '@primer/react'
3+
import type {ForwardRefComponent} from '../polymorphic'
4+
import {sx, type SxProp} from '../sx'
5+
import styled from 'styled-components'
6+
7+
type HeadingLevels = 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6'
8+
9+
type HeadingProps = PrimerHeadingProps & SxProp
10+
11+
const Heading: ForwardRefComponent<HeadingLevels, HeadingProps> = styled(PrimerHeading).withConfig({
12+
shouldForwardProp: prop => (prop as keyof HeadingProps) !== 'sx',
13+
})<HeadingProps>`
14+
${sx}
15+
`
16+
17+
export {Heading, type HeadingProps}

‎packages/styled-react/src/index.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ export {Box, type BoxProps} from './components/Box'
77
export {Button} from '@primer/react'
88
export {CheckboxGroup} from '@primer/react'
99
export {Details} from '@primer/react'
10-
export {Heading} from '@primer/react'
1110
export {IconButton} from '@primer/react'
1211
export {Label} from '@primer/react'
1312
export {Overlay} from '@primer/react'
@@ -36,6 +35,7 @@ export {Dialog, type DialogProps} from './components/Dialog'
3635
export {Flash} from './components/Flash'
3736
export {FormControl, type FormControlProps} from './components/FormControl'
3837
export {Header, type HeaderProps} from './components/Header'
38+
export {Heading} from './components/Heading'
3939
export {Link, type LinkProps} from './components/Link'
4040
export {LinkButton, type LinkButtonProps} from './components/LinkButton'
4141
export {NavList, type NavListProps} from './components/NavList'

0 commit comments

Comments
 (0)