Skip to content

Commit

Permalink
1572 accessibility request primer react heading enforce what as can b…
Browse files Browse the repository at this point in the history
…e set to with a runtime validation rule (#2768)

* fix(Heading): as prop fix, testing type and logging

* chore: add another changeset

* Delete small-bikes-carry.md

Not needed

* chore(Heading): resolved type check error from test
  • Loading branch information
green6erry authored Jan 30, 2023
1 parent 857d34b commit 5055b91
Show file tree
Hide file tree
Showing 4 changed files with 58 additions and 1 deletion.
5 changes: 5 additions & 0 deletions .changeset/fifty-dolls-speak.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': minor
---

Confine Heading as prop to header element types
40 changes: 39 additions & 1 deletion src/Heading.tsx
Original file line number Diff line number Diff line change
@@ -1,14 +1,52 @@
import React, {forwardRef, useEffect} from 'react'
import styled from 'styled-components'
import {get} from './constants'
import {useRefObjectAsForwardedRef} from './hooks'
import sx, {SxProp} from './sx'
import {ComponentProps} from './utils/types'
import {ForwardRefComponent as PolymorphicForwardRefComponent} from './utils/polymorphic'

const Heading = styled.h2<SxProp>`
type StyledHeadingProps = {
as?: 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6'
} & SxProp

const StyledHeading = styled.h2<StyledHeadingProps>`
font-weight: ${get('fontWeights.bold')};
font-size: ${get('fontSizes.5')};
margin: 0;
${sx};
`
const Heading = forwardRef(({as: Component = 'h2', ...props}, forwardedRef) => {
const innerRef = React.useRef<HTMLHeadingElement>(null)
useRefObjectAsForwardedRef(forwardedRef, innerRef)

if (__DEV__) {
/**
* The Linter yells because it thinks this conditionally calls an effect,
* but since this is a compile-time flag and not a runtime conditional
* this is safe, and ensures the entire effect is kept out of prod builds
* shaving precious bytes from the output, and avoiding mounting a noop effect
*/
// eslint-disable-next-line react-hooks/rules-of-hooks
useEffect(() => {
if (innerRef.current && !(innerRef.current instanceof HTMLHeadingElement)) {
// eslint-disable-next-line no-console
console.warn('This Heading component should be an instanceof of h1-h6')
}
}, [innerRef])
}

return (
<StyledHeading
as={Component}
{...props}
// @ts-ignore shh
ref={innerRef}
/>
)
}) as PolymorphicForwardRefComponent<'h2', StyledHeadingProps>

Heading.displayName = 'Heading'

export type HeadingProps = ComponentProps<typeof Heading>
export default Heading
9 changes: 9 additions & 0 deletions src/__tests__/Heading.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,15 @@ describe('Heading', () => {
).toHaveStyleRule('font-size', `${fontSize}`)
}
})
it('logs a warning when trying to render invalid "as" prop', () => {
const consoleSpy = jest.spyOn(global.console, 'warn').mockImplementation()

// @ts-expect-error as prop should not be accepted
HTMLRender(<Heading as="i" />)
expect(consoleSpy).toHaveBeenCalled()

consoleSpy.mockRestore()
})

it('respects the "fontStyle" prop', () => {
expect(
Expand Down
5 changes: 5 additions & 0 deletions src/__tests__/Heading.types.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,8 @@ export function shouldNotAcceptSystemProps() {
// @ts-expect-error system props should not be accepted
return <Heading backgroundColor="thistle" />
}

export function shouldNotAcceptInvalidAsProp() {
// @ts-expect-error as prop should not be accepted
return <Heading as="p" />
}

0 comments on commit 5055b91

Please sign in to comment.