From c791a506a4374a810ec4e0917413a34ed7dfd123 Mon Sep 17 00:00:00 2001 From: kendallgassner Date: Tue, 6 Dec 2022 12:47:23 -0800 Subject: [PATCH 01/10] Refine as prop in button component and fix aria type --- package-lock.json | 31 +++++++++++++++++++++++-------- package.json | 1 + src/Button/ButtonBase.tsx | 24 ++++++++++++++++++------ src/Button/types.ts | 4 +++- 4 files changed, 45 insertions(+), 15 deletions(-) diff --git a/package-lock.json b/package-lock.json index 6d528ae3c24..57bd988d123 100644 --- a/package-lock.json +++ b/package-lock.json @@ -32,6 +32,7 @@ "fzy.js": "0.4.1", "history": "^5.0.0", "react-intersection-observer": "9.4.1", + "react-merge-refs": "2.0.1", "styled-system": "^5.1.5" }, "devDependencies": { @@ -2865,6 +2866,16 @@ "node": ">=6.9.0" } }, + "node_modules/@devtools-ds/themes/node_modules/react-merge-refs": { + "version": "1.1.0", + "resolved": "https://registry.npmjs.org/react-merge-refs/-/react-merge-refs-1.1.0.tgz", + "integrity": "sha512-alTKsjEL0dKH/ru1Iyn7vliS2QRcBp9zZPGoWxUOvRGWPUYgjo+V01is7p04It6KhgrzhJGnIj9GgX8W4bZoCQ==", + "dev": true, + "funding": { + "type": "github", + "url": "https://github.com/sponsors/gregberge" + } + }, "node_modules/@devtools-ds/tree": { "version": "1.2.0", "resolved": "https://registry.npmjs.org/@devtools-ds/tree/-/tree-1.2.0.tgz", @@ -37417,10 +37428,9 @@ "integrity": "sha512-24e6ynE2H+OKt4kqsOvNd8kBpV65zoxbA4BVsEOB3ARVWQki/DHzaUoC5KuON/BiccDaCCTZBuOcfZs70kR8bQ==" }, "node_modules/react-merge-refs": { - "version": "1.1.0", - "resolved": "https://registry.npmjs.org/react-merge-refs/-/react-merge-refs-1.1.0.tgz", - "integrity": "sha512-alTKsjEL0dKH/ru1Iyn7vliS2QRcBp9zZPGoWxUOvRGWPUYgjo+V01is7p04It6KhgrzhJGnIj9GgX8W4bZoCQ==", - "dev": true, + "version": "2.0.1", + "resolved": "https://registry.npmjs.org/react-merge-refs/-/react-merge-refs-2.0.1.tgz", + "integrity": "sha512-pywF6oouJWuqL26xV3OruRSIqai31R9SdJX/I3gP2q8jLxUnA1IwXcLW8werUHLZOrp4N7YOeQNZrh/BKrHI4A==", "funding": { "type": "github", "url": "https://github.com/sponsors/gregberge" @@ -44703,6 +44713,12 @@ } } } + }, + "react-merge-refs": { + "version": "1.1.0", + "resolved": "https://registry.npmjs.org/react-merge-refs/-/react-merge-refs-1.1.0.tgz", + "integrity": "sha512-alTKsjEL0dKH/ru1Iyn7vliS2QRcBp9zZPGoWxUOvRGWPUYgjo+V01is7p04It6KhgrzhJGnIj9GgX8W4bZoCQ==", + "dev": true } } }, @@ -70958,10 +70974,9 @@ "integrity": "sha512-24e6ynE2H+OKt4kqsOvNd8kBpV65zoxbA4BVsEOB3ARVWQki/DHzaUoC5KuON/BiccDaCCTZBuOcfZs70kR8bQ==" }, "react-merge-refs": { - "version": "1.1.0", - "resolved": "https://registry.npmjs.org/react-merge-refs/-/react-merge-refs-1.1.0.tgz", - "integrity": "sha512-alTKsjEL0dKH/ru1Iyn7vliS2QRcBp9zZPGoWxUOvRGWPUYgjo+V01is7p04It6KhgrzhJGnIj9GgX8W4bZoCQ==", - "dev": true + "version": "2.0.1", + "resolved": "https://registry.npmjs.org/react-merge-refs/-/react-merge-refs-2.0.1.tgz", + "integrity": "sha512-pywF6oouJWuqL26xV3OruRSIqai31R9SdJX/I3gP2q8jLxUnA1IwXcLW8werUHLZOrp4N7YOeQNZrh/BKrHI4A==" }, "react-refresh": { "version": "0.11.0", diff --git a/package.json b/package.json index 32d3cac7335..eb4dae617c3 100644 --- a/package.json +++ b/package.json @@ -104,6 +104,7 @@ "fzy.js": "0.4.1", "history": "^5.0.0", "react-intersection-observer": "9.4.1", + "react-merge-refs": "2.0.1", "styled-system": "^5.1.5" }, "devDependencies": { diff --git a/src/Button/ButtonBase.tsx b/src/Button/ButtonBase.tsx index 01840e098d6..dff71694666 100644 --- a/src/Button/ButtonBase.tsx +++ b/src/Button/ButtonBase.tsx @@ -1,5 +1,6 @@ import React, {ComponentPropsWithRef, forwardRef, useMemo} from 'react' -import {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' +import {mergeRefs} from 'react-merge-refs' +import {ForwardRefComponent as PolymorphicForwardRefComponent, IntrinsicElement} from '../utils/polymorphic' import Box from '../Box' import {merge, SxProp} from '../sx' import {useTheme} from '../ThemeProvider' @@ -8,16 +9,17 @@ import {getVariantStyles, getSizeStyles, getButtonStyles} from './styles' const defaultSxProp = {} const iconWrapStyles = { - display: 'inline-block', + display: 'inline-block' } const trailingIconStyles = { ...iconWrapStyles, - ml: 2, + ml: 2 } const ButtonBase = forwardRef( ({children, as: Component = 'button', sx: sxProp = defaultSxProp, ...props}, forwardedRef): JSX.Element => { const {leadingIcon: LeadingIcon, trailingIcon: TrailingIcon, variant = 'default', size = 'medium', ...rest} = props + const innerRef = React.useRef() const {theme} = useTheme() const baseStyles = useMemo(() => { return merge.all([getButtonStyles(theme), getSizeStyles(size, variant, false), getVariantStyles(variant, theme)]) @@ -26,8 +28,18 @@ const ButtonBase = forwardRef( return merge(baseStyles, sxProp as SxProp) }, [baseStyles, sxProp]) + React.useEffect(() => { + if ( + innerRef && + !(innerRef.current instanceof HTMLButtonElement) && + !(innerRef.current instanceof HTMLAnchorElement) + ) { + console.warn('This component should be an instanceof a semantic button or anchor') + } + }, [innerRef]) + return ( - + {LeadingIcon && ( @@ -41,8 +53,8 @@ const ButtonBase = forwardRef( )} ) - }, -) as PolymorphicForwardRefComponent<'button' | 'a', ButtonProps> + } +) as PolymorphicForwardRefComponent | IntrinsicElement<'a'>, ButtonProps> export type ButtonBaseProps = ComponentPropsWithRef diff --git a/src/Button/types.ts b/src/Button/types.ts index 629809b6f1b..4072002d332 100644 --- a/src/Button/types.ts +++ b/src/Button/types.ts @@ -18,7 +18,9 @@ export type Size = 'small' | 'medium' | 'large' */ type StyledButtonProps = Omit, 'as'> -type ButtonA11yProps = {'aria-label': string; 'aria-labelby'?: never} | {'aria-label'?: never; 'aria-labelby': string} +type ButtonA11yProps = + | {'aria-label': string; 'aria-labelledby'?: never} + | {'aria-label'?: never; 'aria-labelledby': string} export type ButtonBaseProps = { /** From 57a6fc6c89ca86db04f01cc2be52c5ca54cb8f2e Mon Sep 17 00:00:00 2001 From: kendallgassner Date: Tue, 6 Dec 2022 12:56:37 -0800 Subject: [PATCH 02/10] whoops, remove type change --- src/Button/ButtonBase.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Button/ButtonBase.tsx b/src/Button/ButtonBase.tsx index dff71694666..d797fe1ab73 100644 --- a/src/Button/ButtonBase.tsx +++ b/src/Button/ButtonBase.tsx @@ -54,7 +54,7 @@ const ButtonBase = forwardRef( ) } -) as PolymorphicForwardRefComponent | IntrinsicElement<'a'>, ButtonProps> +) as PolymorphicForwardRefComponent<'button' | 'a', ButtonProps> export type ButtonBaseProps = ComponentPropsWithRef From 91e89e9edc947ce9f316b4eec5dc2b95a4cea13d Mon Sep 17 00:00:00 2001 From: kendallgassner Date: Tue, 6 Dec 2022 13:05:49 -0800 Subject: [PATCH 03/10] fix lint bugs --- src/Button/ButtonBase.tsx | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/Button/ButtonBase.tsx b/src/Button/ButtonBase.tsx index d797fe1ab73..62d846a4a3d 100644 --- a/src/Button/ButtonBase.tsx +++ b/src/Button/ButtonBase.tsx @@ -1,6 +1,6 @@ import React, {ComponentPropsWithRef, forwardRef, useMemo} from 'react' import {mergeRefs} from 'react-merge-refs' -import {ForwardRefComponent as PolymorphicForwardRefComponent, IntrinsicElement} from '../utils/polymorphic' +import {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' import Box from '../Box' import {merge, SxProp} from '../sx' import {useTheme} from '../ThemeProvider' @@ -9,11 +9,11 @@ import {getVariantStyles, getSizeStyles, getButtonStyles} from './styles' const defaultSxProp = {} const iconWrapStyles = { - display: 'inline-block' + display: 'inline-block', } const trailingIconStyles = { ...iconWrapStyles, - ml: 2 + ml: 2, } const ButtonBase = forwardRef( @@ -29,11 +29,8 @@ const ButtonBase = forwardRef( }, [baseStyles, sxProp]) React.useEffect(() => { - if ( - innerRef && - !(innerRef.current instanceof HTMLButtonElement) && - !(innerRef.current instanceof HTMLAnchorElement) - ) { + if (!(innerRef.current instanceof HTMLButtonElement) && !(innerRef.current instanceof HTMLAnchorElement)) { + // eslint-disable-next-line no-console console.warn('This component should be an instanceof a semantic button or anchor') } }, [innerRef]) @@ -53,7 +50,7 @@ const ButtonBase = forwardRef( )} ) - } + }, ) as PolymorphicForwardRefComponent<'button' | 'a', ButtonProps> export type ButtonBaseProps = ComponentPropsWithRef From e8036cba05762f3ebbac816cd63d68081289bf64 Mon Sep 17 00:00:00 2001 From: kendallgassner Date: Tue, 6 Dec 2022 15:20:13 -0800 Subject: [PATCH 04/10] changeset --- .changeset/giant-horses-knock.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/giant-horses-knock.md diff --git a/.changeset/giant-horses-knock.md b/.changeset/giant-horses-knock.md new file mode 100644 index 00000000000..2a72dbe84aa --- /dev/null +++ b/.changeset/giant-horses-knock.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +Add a console warning if the Button and IconButton as property is used incorrectly From d28127f9ce642428b17475625124762bf0fc3fae Mon Sep 17 00:00:00 2001 From: kendallgassner Date: Wed, 7 Dec 2022 09:50:10 -0800 Subject: [PATCH 05/10] use local mergeRef --- package-lock.json | 15 --------------- package.json | 1 - src/Button/ButtonBase.tsx | 10 ++++++---- 3 files changed, 6 insertions(+), 20 deletions(-) diff --git a/package-lock.json b/package-lock.json index 57bd988d123..0cc06237cb7 100644 --- a/package-lock.json +++ b/package-lock.json @@ -32,7 +32,6 @@ "fzy.js": "0.4.1", "history": "^5.0.0", "react-intersection-observer": "9.4.1", - "react-merge-refs": "2.0.1", "styled-system": "^5.1.5" }, "devDependencies": { @@ -37427,15 +37426,6 @@ "resolved": "https://registry.npmjs.org/react-is/-/react-is-16.13.1.tgz", "integrity": "sha512-24e6ynE2H+OKt4kqsOvNd8kBpV65zoxbA4BVsEOB3ARVWQki/DHzaUoC5KuON/BiccDaCCTZBuOcfZs70kR8bQ==" }, - "node_modules/react-merge-refs": { - "version": "2.0.1", - "resolved": "https://registry.npmjs.org/react-merge-refs/-/react-merge-refs-2.0.1.tgz", - "integrity": "sha512-pywF6oouJWuqL26xV3OruRSIqai31R9SdJX/I3gP2q8jLxUnA1IwXcLW8werUHLZOrp4N7YOeQNZrh/BKrHI4A==", - "funding": { - "type": "github", - "url": "https://github.com/sponsors/gregberge" - } - }, "node_modules/react-refresh": { "version": "0.11.0", "resolved": "https://registry.npmjs.org/react-refresh/-/react-refresh-0.11.0.tgz", @@ -70973,11 +70963,6 @@ "resolved": "https://registry.npmjs.org/react-is/-/react-is-16.13.1.tgz", "integrity": "sha512-24e6ynE2H+OKt4kqsOvNd8kBpV65zoxbA4BVsEOB3ARVWQki/DHzaUoC5KuON/BiccDaCCTZBuOcfZs70kR8bQ==" }, - "react-merge-refs": { - "version": "2.0.1", - "resolved": "https://registry.npmjs.org/react-merge-refs/-/react-merge-refs-2.0.1.tgz", - "integrity": "sha512-pywF6oouJWuqL26xV3OruRSIqai31R9SdJX/I3gP2q8jLxUnA1IwXcLW8werUHLZOrp4N7YOeQNZrh/BKrHI4A==" - }, "react-refresh": { "version": "0.11.0", "resolved": "https://registry.npmjs.org/react-refresh/-/react-refresh-0.11.0.tgz", diff --git a/package.json b/package.json index eb4dae617c3..32d3cac7335 100644 --- a/package.json +++ b/package.json @@ -104,7 +104,6 @@ "fzy.js": "0.4.1", "history": "^5.0.0", "react-intersection-observer": "9.4.1", - "react-merge-refs": "2.0.1", "styled-system": "^5.1.5" }, "devDependencies": { diff --git a/src/Button/ButtonBase.tsx b/src/Button/ButtonBase.tsx index 62d846a4a3d..50477db205d 100644 --- a/src/Button/ButtonBase.tsx +++ b/src/Button/ButtonBase.tsx @@ -1,11 +1,11 @@ -import React, {ComponentPropsWithRef, forwardRef, useMemo} from 'react' -import {mergeRefs} from 'react-merge-refs' +import React, {ComponentPropsWithRef, ForwardedRef, forwardRef, useMemo} from 'react' import {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' import Box from '../Box' import {merge, SxProp} from '../sx' import {useTheme} from '../ThemeProvider' import {ButtonProps, StyledButton} from './types' import {getVariantStyles, getSizeStyles, getButtonStyles} from './styles' +import {useRefObjectAsForwardedRef} from '../hooks/useRefObjectAsForwardedRef' const defaultSxProp = {} const iconWrapStyles = { @@ -19,7 +19,9 @@ const trailingIconStyles = { const ButtonBase = forwardRef( ({children, as: Component = 'button', sx: sxProp = defaultSxProp, ...props}, forwardedRef): JSX.Element => { const {leadingIcon: LeadingIcon, trailingIcon: TrailingIcon, variant = 'default', size = 'medium', ...rest} = props - const innerRef = React.useRef() + const innerRef = React.useRef(null) + useRefObjectAsForwardedRef(forwardedRef, innerRef) + const {theme} = useTheme() const baseStyles = useMemo(() => { return merge.all([getButtonStyles(theme), getSizeStyles(size, variant, false), getVariantStyles(variant, theme)]) @@ -36,7 +38,7 @@ const ButtonBase = forwardRef( }, [innerRef]) return ( - + {LeadingIcon && ( From d4ab06e031797a23e188e51fa6f91ad3a22e8c45 Mon Sep 17 00:00:00 2001 From: kendallgassner Date: Wed, 7 Dec 2022 09:51:17 -0800 Subject: [PATCH 06/10] undo package-lock changes --- package-lock.json | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/package-lock.json b/package-lock.json index 0cc06237cb7..6d528ae3c24 100644 --- a/package-lock.json +++ b/package-lock.json @@ -2865,16 +2865,6 @@ "node": ">=6.9.0" } }, - "node_modules/@devtools-ds/themes/node_modules/react-merge-refs": { - "version": "1.1.0", - "resolved": "https://registry.npmjs.org/react-merge-refs/-/react-merge-refs-1.1.0.tgz", - "integrity": "sha512-alTKsjEL0dKH/ru1Iyn7vliS2QRcBp9zZPGoWxUOvRGWPUYgjo+V01is7p04It6KhgrzhJGnIj9GgX8W4bZoCQ==", - "dev": true, - "funding": { - "type": "github", - "url": "https://github.com/sponsors/gregberge" - } - }, "node_modules/@devtools-ds/tree": { "version": "1.2.0", "resolved": "https://registry.npmjs.org/@devtools-ds/tree/-/tree-1.2.0.tgz", @@ -37426,6 +37416,16 @@ "resolved": "https://registry.npmjs.org/react-is/-/react-is-16.13.1.tgz", "integrity": "sha512-24e6ynE2H+OKt4kqsOvNd8kBpV65zoxbA4BVsEOB3ARVWQki/DHzaUoC5KuON/BiccDaCCTZBuOcfZs70kR8bQ==" }, + "node_modules/react-merge-refs": { + "version": "1.1.0", + "resolved": "https://registry.npmjs.org/react-merge-refs/-/react-merge-refs-1.1.0.tgz", + "integrity": "sha512-alTKsjEL0dKH/ru1Iyn7vliS2QRcBp9zZPGoWxUOvRGWPUYgjo+V01is7p04It6KhgrzhJGnIj9GgX8W4bZoCQ==", + "dev": true, + "funding": { + "type": "github", + "url": "https://github.com/sponsors/gregberge" + } + }, "node_modules/react-refresh": { "version": "0.11.0", "resolved": "https://registry.npmjs.org/react-refresh/-/react-refresh-0.11.0.tgz", @@ -44703,12 +44703,6 @@ } } } - }, - "react-merge-refs": { - "version": "1.1.0", - "resolved": "https://registry.npmjs.org/react-merge-refs/-/react-merge-refs-1.1.0.tgz", - "integrity": "sha512-alTKsjEL0dKH/ru1Iyn7vliS2QRcBp9zZPGoWxUOvRGWPUYgjo+V01is7p04It6KhgrzhJGnIj9GgX8W4bZoCQ==", - "dev": true } } }, @@ -70963,6 +70957,12 @@ "resolved": "https://registry.npmjs.org/react-is/-/react-is-16.13.1.tgz", "integrity": "sha512-24e6ynE2H+OKt4kqsOvNd8kBpV65zoxbA4BVsEOB3ARVWQki/DHzaUoC5KuON/BiccDaCCTZBuOcfZs70kR8bQ==" }, + "react-merge-refs": { + "version": "1.1.0", + "resolved": "https://registry.npmjs.org/react-merge-refs/-/react-merge-refs-1.1.0.tgz", + "integrity": "sha512-alTKsjEL0dKH/ru1Iyn7vliS2QRcBp9zZPGoWxUOvRGWPUYgjo+V01is7p04It6KhgrzhJGnIj9GgX8W4bZoCQ==", + "dev": true + }, "react-refresh": { "version": "0.11.0", "resolved": "https://registry.npmjs.org/react-refresh/-/react-refresh-0.11.0.tgz", From cc8e2d015175c4bfca9b2505e1714807f0f6c4e3 Mon Sep 17 00:00:00 2001 From: Kendall Gassner Date: Wed, 7 Dec 2022 09:56:34 -0800 Subject: [PATCH 07/10] Update src/Button/ButtonBase.tsx --- src/Button/ButtonBase.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Button/ButtonBase.tsx b/src/Button/ButtonBase.tsx index 50477db205d..4dc99d8be9f 100644 --- a/src/Button/ButtonBase.tsx +++ b/src/Button/ButtonBase.tsx @@ -1,4 +1,4 @@ -import React, {ComponentPropsWithRef, ForwardedRef, forwardRef, useMemo} from 'react' +import React, {ComponentPropsWithRef, forwardRef, useMemo} from 'react' import {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' import Box from '../Box' import {merge, SxProp} from '../sx' From a0da7386d3ac771ae73ec1162202e0f79fe3b9ca Mon Sep 17 00:00:00 2001 From: Kendall Gassner Date: Wed, 7 Dec 2022 10:35:11 -0800 Subject: [PATCH 08/10] Update src/Button/ButtonBase.tsx Co-authored-by: Josh Black --- src/Button/ButtonBase.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Button/ButtonBase.tsx b/src/Button/ButtonBase.tsx index 4dc99d8be9f..91af85286ce 100644 --- a/src/Button/ButtonBase.tsx +++ b/src/Button/ButtonBase.tsx @@ -33,7 +33,9 @@ const ButtonBase = forwardRef( React.useEffect(() => { if (!(innerRef.current instanceof HTMLButtonElement) && !(innerRef.current instanceof HTMLAnchorElement)) { // eslint-disable-next-line no-console - console.warn('This component should be an instanceof a semantic button or anchor') + if (__DEV__) { + console.warn('This component should be an instanceof a semantic button or anchor') + } } }, [innerRef]) From 17e6150a428b3c0068d67c539c47859076442e82 Mon Sep 17 00:00:00 2001 From: kendallgassner Date: Wed, 7 Dec 2022 10:53:20 -0800 Subject: [PATCH 09/10] fix eslint --- src/Button/ButtonBase.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Button/ButtonBase.tsx b/src/Button/ButtonBase.tsx index 91af85286ce..7ab1ac3fd5d 100644 --- a/src/Button/ButtonBase.tsx +++ b/src/Button/ButtonBase.tsx @@ -32,8 +32,8 @@ const ButtonBase = forwardRef( React.useEffect(() => { if (!(innerRef.current instanceof HTMLButtonElement) && !(innerRef.current instanceof HTMLAnchorElement)) { - // eslint-disable-next-line no-console if (__DEV__) { + // eslint-disable-next-line no-console console.warn('This component should be an instanceof a semantic button or anchor') } } From 7f32cfc8faaeca80c1323f3dad1c482b0ac25838 Mon Sep 17 00:00:00 2001 From: kendallgassner Date: Wed, 7 Dec 2022 11:10:44 -0800 Subject: [PATCH 10/10] define dev --- src/Button/ButtonBase.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Button/ButtonBase.tsx b/src/Button/ButtonBase.tsx index 7ab1ac3fd5d..6c2894400d1 100644 --- a/src/Button/ButtonBase.tsx +++ b/src/Button/ButtonBase.tsx @@ -6,6 +6,7 @@ import {useTheme} from '../ThemeProvider' import {ButtonProps, StyledButton} from './types' import {getVariantStyles, getSizeStyles, getButtonStyles} from './styles' import {useRefObjectAsForwardedRef} from '../hooks/useRefObjectAsForwardedRef' +declare let __DEV__: boolean const defaultSxProp = {} const iconWrapStyles = {