From 37ba4ef85ec812a9c710b2d4ad1c3cbdca10e66f Mon Sep 17 00:00:00 2001 From: "Amanda G. Brown" Date: Tue, 24 Jan 2023 11:30:46 -0500 Subject: [PATCH 1/4] fix(Heading): as prop fix, testing type and logging --- .changeset/small-bikes-carry.md | 5 ++++ src/Heading.tsx | 40 +++++++++++++++++++++++++++- src/__tests__/Heading.test.tsx | 8 ++++++ src/__tests__/Heading.types.test.tsx | 5 ++++ 4 files changed, 57 insertions(+), 1 deletion(-) create mode 100644 .changeset/small-bikes-carry.md diff --git a/.changeset/small-bikes-carry.md b/.changeset/small-bikes-carry.md new file mode 100644 index 00000000000..5aa965233d9 --- /dev/null +++ b/.changeset/small-bikes-carry.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..21b8b4b8f46 100644 --- a/src/__tests__/Heading.test.tsx +++ b/src/__tests__/Heading.test.tsx @@ -122,6 +122,14 @@ 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() + + 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 +} From b2dd8fe857c3b946e6aabf9aa8dc8553e2d98bc5 Mon Sep 17 00:00:00 2001 From: "Amanda G. Brown" Date: Tue, 24 Jan 2023 11:40:29 -0500 Subject: [PATCH 2/4] chore: add another changeset --- .changeset/fifty-dolls-speak.md | 5 +++++ 1 file changed, 5 insertions(+) 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 From d351b69a3b3fc643d6dd19ca35188a20f8f6b698 Mon Sep 17 00:00:00 2001 From: Amanda Brown Date: Tue, 24 Jan 2023 11:41:56 -0500 Subject: [PATCH 3/4] Delete small-bikes-carry.md Not needed --- .changeset/small-bikes-carry.md | 5 ----- 1 file changed, 5 deletions(-) delete mode 100644 .changeset/small-bikes-carry.md diff --git a/.changeset/small-bikes-carry.md b/.changeset/small-bikes-carry.md deleted file mode 100644 index 5aa965233d9..00000000000 --- a/.changeset/small-bikes-carry.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -'@primer/react': minor ---- - -Confine Heading as prop to header element types From 4f1746c71b5d1fa0caa474e9b23a13059cee28bd Mon Sep 17 00:00:00 2001 From: "Amanda G. Brown" Date: Tue, 24 Jan 2023 12:47:06 -0500 Subject: [PATCH 4/4] chore(Heading): resolved type check error from test --- src/__tests__/Heading.test.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/src/__tests__/Heading.test.tsx b/src/__tests__/Heading.test.tsx index 21b8b4b8f46..f9cb751ac0f 100644 --- a/src/__tests__/Heading.test.tsx +++ b/src/__tests__/Heading.test.tsx @@ -125,6 +125,7 @@ describe('Heading', () => { 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()