From 9cc833f85bf442d4c826fa21c07710afae2553b3 Mon Sep 17 00:00:00 2001 From: James Mockett <1166188+jamesmockett@users.noreply.github.com> Date: Thu, 13 Jun 2024 15:05:32 +0100 Subject: [PATCH 1/5] Set icon appearance using props rather than overriding styles --- .../src/react-components/button/Button.tsx | 1 + .../src/react-components/button/shared.tsx | 46 ++++++++++++++----- .../src/react-components/button/styles.ts | 31 +------------ 3 files changed, 36 insertions(+), 42 deletions(-) diff --git a/libs/@guardian/source/src/react-components/button/Button.tsx b/libs/@guardian/source/src/react-components/button/Button.tsx index 0817428ca..c41baed1f 100644 --- a/libs/@guardian/source/src/react-components/button/Button.tsx +++ b/libs/@guardian/source/src/react-components/button/Button.tsx @@ -49,6 +49,7 @@ export const Button = ({ {...props} > {buttonContents({ + size, hideLabel, iconSvg, isLoading, diff --git a/libs/@guardian/source/src/react-components/button/shared.tsx b/libs/@guardian/source/src/react-components/button/shared.tsx index 1b29970f2..fdfa70d86 100644 --- a/libs/@guardian/source/src/react-components/button/shared.tsx +++ b/libs/@guardian/source/src/react-components/button/shared.tsx @@ -3,13 +3,32 @@ import type { ReactElement, ReactNode } from 'react'; import { cloneElement } from 'react'; import { visuallyHidden } from '../../foundations'; import { Spinner } from '../spinner/Spinner'; +import { IconProps, IconSize } from '../@types/Icons'; +import { Size } from './@types/SharedButtonProps'; + +const iconSize: Record = { + xsmall: 'xsmall', + small: 'small', + default: 'medium', +}; + +/** + * Spinners do not use the same icon sizes so we specify custom sizes in pixels + */ +const spinnerSize: Record = { + xsmall: 16, + small: 20, + default: 24, +}; export const buttonContents = ({ + size = 'default', hideLabel, iconSvg, isLoading, children, }: { + size?: Size; hideLabel?: boolean; iconSvg?: ReactElement; isLoading?: boolean; @@ -22,23 +41,26 @@ export const buttonContents = ({ contents.push(
); } contents.push( - cloneElement( - , - { - key: 'svg', - }, - ), + , ); } else if (iconSvg) { if (!hideLabel) { contents.push(
); } - contents.push(cloneElement(iconSvg, { key: 'svg' })); + contents.push( + cloneElement(iconSvg as ReactElement, { + size: iconSize[size], + theme: { fill: 'currentColor' }, + key: 'icon', + }), + ); } if (hideLabel) { return ( diff --git a/libs/@guardian/source/src/react-components/button/styles.ts b/libs/@guardian/source/src/react-components/button/styles.ts index d90942f47..17d918211 100644 --- a/libs/@guardian/source/src/react-components/button/styles.ts +++ b/libs/@guardian/source/src/react-components/button/styles.ts @@ -42,26 +42,6 @@ const button = css` } `; -// Width of the loading spinner in pixels for each button size. -const loadingSpinnerSizes: Record = { - xsmall: 16, - small: 20, - default: 24, -}; - -const applyButtonStylesToLoadingSpinner = (size: Size) => { - return css` - svg { - /* - * The loading spinner width has been specified as 24px in the design - * which falls outside of the icon sizes in foundations, so we - * override the width here. - */ - width: ${loadingSpinnerSizes[size]}px; - } - `; -}; - const primary = (button: ThemeButton): SerializedStyles => css` background-color: ${button.backgroundPrimary}; color: ${button.textPrimary}; @@ -143,10 +123,7 @@ const iconDefault = css` svg { flex: 0 0 auto; display: block; - fill: currentColor; position: relative; - width: ${width.iconMedium}px; - height: auto; } .src-button-space { width: ${space[3]}px; @@ -157,10 +134,7 @@ const iconSmall = css` svg { flex: 0 0 auto; display: block; - fill: currentColor; position: relative; - width: ${width.iconSmall}px; - height: auto; } .src-button-space { width: ${space[2]}px; @@ -171,10 +145,7 @@ const iconXsmall = css` svg { flex: 0 0 auto; display: block; - fill: currentColor; position: relative; - width: ${width.iconXsmall}px; - height: auto; } .src-button-space { width: ${space[1]}px; @@ -193,6 +164,7 @@ const iconLeft = css` margin-left: ${pullIconTowardEdge}px; } `; + const iconRight = css` svg { margin-right: ${pullIconTowardEdge}px; @@ -296,6 +268,5 @@ export const buttonStyles = (iconSvg ?? isLoading) && !hideLabel ? iconSides[iconSide] : '', nudgeIcon ? iconNudgeAnimation : '', hideLabel ? iconOnlySizes[size] : '', - isLoading ? applyButtonStylesToLoadingSpinner(size) : undefined, cssOverrides, ]; From d19a29706db8e583273337a980cb14e15542cedb Mon Sep 17 00:00:00 2001 From: James Mockett <1166188+jamesmockett@users.noreply.github.com> Date: Thu, 13 Jun 2024 17:36:30 +0100 Subject: [PATCH 2/5] Update `LinkButton` to pass through `size` prop --- libs/@guardian/source/src/react-components/button/LinkButton.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/libs/@guardian/source/src/react-components/button/LinkButton.tsx b/libs/@guardian/source/src/react-components/button/LinkButton.tsx index 9559f2586..0b9dd378e 100644 --- a/libs/@guardian/source/src/react-components/button/LinkButton.tsx +++ b/libs/@guardian/source/src/react-components/button/LinkButton.tsx @@ -42,6 +42,7 @@ export const LinkButton = ({ {...props} > {buttonContents({ + size, hideLabel, iconSvg, children, From ab51dc4e06867d726b8a907da676477ddb1dd2c4 Mon Sep 17 00:00:00 2001 From: James Mockett <1166188+jamesmockett@users.noreply.github.com> Date: Thu, 13 Jun 2024 17:44:13 +0100 Subject: [PATCH 3/5] Omit unsupported loading props from `LinkButton` --- .../@guardian/source/src/react-components/button/LinkButton.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/@guardian/source/src/react-components/button/LinkButton.tsx b/libs/@guardian/source/src/react-components/button/LinkButton.tsx index 0b9dd378e..76c6d2037 100644 --- a/libs/@guardian/source/src/react-components/button/LinkButton.tsx +++ b/libs/@guardian/source/src/react-components/button/LinkButton.tsx @@ -4,7 +4,7 @@ import { buttonContents } from './shared'; import { buttonStyles } from './styles'; export interface LinkButtonProps - extends SharedButtonProps, + extends Omit, AnchorHTMLAttributes {} /** From 3534fe66c9206b765e5017af73640be4df306949 Mon Sep 17 00:00:00 2001 From: James Mockett <1166188+jamesmockett@users.noreply.github.com> Date: Thu, 13 Jun 2024 18:07:25 +0100 Subject: [PATCH 4/5] Replace duplicated icon size styles --- .../src/react-components/button/styles.ts | 69 ++++++------------- 1 file changed, 22 insertions(+), 47 deletions(-) diff --git a/libs/@guardian/source/src/react-components/button/styles.ts b/libs/@guardian/source/src/react-components/button/styles.ts index 17d918211..b987a9949 100644 --- a/libs/@guardian/source/src/react-components/button/styles.ts +++ b/libs/@guardian/source/src/react-components/button/styles.ts @@ -40,6 +40,12 @@ const button = css` &:focus { ${focusHaloSpaced}; } + + svg { + flex: 0 0 auto; + display: block; + position: relative; + } `; const primary = (button: ThemeButton): SerializedStyles => css` @@ -99,6 +105,10 @@ const defaultSize = css` padding: 0 ${space[5]}px; border-radius: ${height.ctaMedium}px; padding-bottom: 2px; + + .src-button-space { + width: ${space[3]}px; + } `; const smallSize = css` @@ -108,6 +118,10 @@ const smallSize = css` padding: 0 ${space[4]}px; border-radius: ${height.ctaSmall}px; padding-bottom: 2px; + + .src-button-space { + width: ${space[2]}px; + } `; const xsmallSize = css` @@ -117,44 +131,16 @@ const xsmallSize = css` padding: 0 ${space[3]}px; border-radius: ${height.ctaXsmall}px; padding-bottom: 1px; -`; - -const iconDefault = css` - svg { - flex: 0 0 auto; - display: block; - position: relative; - } - .src-button-space { - width: ${space[3]}px; - } -`; -const iconSmall = css` - svg { - flex: 0 0 auto; - display: block; - position: relative; - } - .src-button-space { - width: ${space[2]}px; - } -`; - -const iconXsmall = css` - svg { - flex: 0 0 auto; - display: block; - position: relative; - } .src-button-space { width: ${space[1]}px; } `; -/* TODO: we add some negative margin to icons to account for - the extra space encoded into the SVG. We should consider removing - or significantly reducing this space +/** + * TODO: we add some negative margin to icons to account for the extra space + * encoded into the SVG. We should consider removing or significantly reducing + * this space. */ const pullIconTowardEdge = -space[1]; @@ -171,22 +157,18 @@ const iconRight = css` } `; -const iconOnly = css` - padding: 0; -`; - const iconOnlyDefault = css` - ${iconOnly}; + padding: 0; width: ${width.ctaMedium}px; `; const iconOnlySmall = css` - ${iconOnly}; + padding: 0; width: ${width.ctaSmall}px; `; const iconOnlyXsmall = css` - ${iconOnly}; + padding: 0; width: ${width.ctaXsmall}px; `; @@ -226,13 +208,7 @@ const sizes: { small: smallSize, xsmall: xsmallSize, }; -const iconSizes: { - [key in Size]: SerializedStyles; -} = { - default: iconDefault, - small: iconSmall, - xsmall: iconXsmall, -}; + const iconOnlySizes: { [key in Size]: SerializedStyles; } = { @@ -264,7 +240,6 @@ export const buttonStyles = button, sizes[size], priorities[priority](mergedTheme(providerTheme.button, theme)), - iconSvg ?? isLoading ? iconSizes[size] : '', (iconSvg ?? isLoading) && !hideLabel ? iconSides[iconSide] : '', nudgeIcon ? iconNudgeAnimation : '', hideLabel ? iconOnlySizes[size] : '', From fa453641ec77cdd2452bbf5f157ffb9591caf5e2 Mon Sep 17 00:00:00 2001 From: James Mockett <1166188+jamesmockett@users.noreply.github.com> Date: Thu, 13 Jun 2024 18:16:40 +0100 Subject: [PATCH 5/5] Fix import order --- libs/@guardian/source/src/react-components/button/shared.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libs/@guardian/source/src/react-components/button/shared.tsx b/libs/@guardian/source/src/react-components/button/shared.tsx index fdfa70d86..1336cdec6 100644 --- a/libs/@guardian/source/src/react-components/button/shared.tsx +++ b/libs/@guardian/source/src/react-components/button/shared.tsx @@ -2,9 +2,9 @@ import { css } from '@emotion/react'; import type { ReactElement, ReactNode } from 'react'; import { cloneElement } from 'react'; import { visuallyHidden } from '../../foundations'; +import type { IconProps, IconSize } from '../@types/Icons'; import { Spinner } from '../spinner/Spinner'; -import { IconProps, IconSize } from '../@types/Icons'; -import { Size } from './@types/SharedButtonProps'; +import type { Size } from './@types/SharedButtonProps'; const iconSize: Record = { xsmall: 'xsmall',