From 5055b91b9cf40e163b9b19fa4a5bcc707adae329 Mon Sep 17 00:00:00 2001 From: Amanda Brown Date: Mon, 30 Jan 2023 13:22:36 -0500 Subject: [PATCH] 1572 accessibility request primer react heading enforce what as can be 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 --- .changeset/fifty-dolls-speak.md | 5 ++++ src/Heading.tsx | 40 +++++++++++++++++++++++++++- src/__tests__/Heading.test.tsx | 9 +++++++ src/__tests__/Heading.types.test.tsx | 5 ++++ 4 files changed, 58 insertions(+), 1 deletion(-) create mode 100644 .changeset/fifty-dolls-speak.md diff --git a/.changeset/fifty-dolls-speak.md b/.changeset/fifty-dolls-speak.md new file mode 100644 index 00000000000..5aa965233d9 --- /dev/null +++ b/.changeset/fifty-dolls-speak.md @@ -0,0 +1,5 @@ +--- +'@primer/react': minor +--- + +Confine Heading as prop to header element types diff --git a/src/Heading.tsx b/src/Heading.tsx index c6ef73bf3ab..2a2dbf578f3 100644 --- a/src/Heading.tsx +++ b/src/Heading.tsx @@ -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` +type StyledHeadingProps = { + as?: 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6' +} & SxProp + +const StyledHeading = styled.h2` 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(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 ( + + ) +}) as PolymorphicForwardRefComponent<'h2', StyledHeadingProps> + +Heading.displayName = 'Heading' export type HeadingProps = ComponentProps export default Heading diff --git a/src/__tests__/Heading.test.tsx b/src/__tests__/Heading.test.tsx index 49cc03e0b3b..f9cb751ac0f 100644 --- a/src/__tests__/Heading.test.tsx +++ b/src/__tests__/Heading.test.tsx @@ -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() + expect(consoleSpy).toHaveBeenCalled() + + consoleSpy.mockRestore() + }) it('respects the "fontStyle" prop', () => { expect( diff --git a/src/__tests__/Heading.types.test.tsx b/src/__tests__/Heading.types.test.tsx index 09a79a8bfee..49d9db3b8f2 100644 --- a/src/__tests__/Heading.types.test.tsx +++ b/src/__tests__/Heading.types.test.tsx @@ -9,3 +9,8 @@ export function shouldNotAcceptSystemProps() { // @ts-expect-error system props should not be accepted return } + +export function shouldNotAcceptInvalidAsProp() { + // @ts-expect-error as prop should not be accepted + return +}