From 2485ff35abea75a78a65ccb4f9267e75e9a3222d Mon Sep 17 00:00:00 2001 From: Vegard Haugstvedt Date: Fri, 18 Oct 2024 09:45:53 +0200 Subject: [PATCH 01/15] Check loading prop for showing spinner, not widthOverride. Making this change because widthOverride sometimes gets set to 0 (should also be fixed), meaning this check fails as "falsy". --- @navikt/core/react/src/button/Button.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/@navikt/core/react/src/button/Button.tsx b/@navikt/core/react/src/button/Button.tsx index 0f197f5799..d98c23fdd1 100644 --- a/@navikt/core/react/src/button/Button.tsx +++ b/@navikt/core/react/src/button/Button.tsx @@ -132,7 +132,7 @@ export const Button: OverridableComponent = }} disabled={disabled ?? widthOverride ? true : undefined} > - {widthOverride ? ( + {loading ? ( ) : ( <> From e09bab9361cca9d6b7c560a769a32075679e3fb6 Mon Sep 17 00:00:00 2001 From: Vegard Haugstvedt Date: Fri, 18 Oct 2024 09:54:04 +0200 Subject: [PATCH 02/15] Prevent setting width to 0 while showing spinner, to avoid issue in Modals Checking that width is truthy (not zero) will avoid issue where widthOverride gets set to 0 because the button is not yet rendered. --- @navikt/core/react/src/button/Button.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/@navikt/core/react/src/button/Button.tsx b/@navikt/core/react/src/button/Button.tsx index d98c23fdd1..c0e2ef11d9 100644 --- a/@navikt/core/react/src/button/Button.tsx +++ b/@navikt/core/react/src/button/Button.tsx @@ -87,7 +87,7 @@ export const Button: OverridableComponent = const mergedRef = useMergeRefs(buttonRef, ref); useClientLayoutEffect(() => { - if (loading) { + if (loading && buttonRef?.current?.getBoundingClientRect()?.width) { const requestID = window.requestAnimationFrame(() => { setWidthOverride( buttonRef?.current?.getBoundingClientRect()?.width, @@ -98,7 +98,7 @@ export const Button: OverridableComponent = cancelAnimationFrame(requestID); }; } - }, [loading, children]); + }, [loading, children, buttonRef?.current]); const filterProps: React.ButtonHTMLAttributes = disabled ?? widthOverride ? omit(rest, ["href"]) : rest; From 1d500d7ff9d59a8036d572be3c83a9b5cf01a062 Mon Sep 17 00:00:00 2001 From: Vegard Haugstvedt Date: Fri, 18 Oct 2024 10:01:12 +0200 Subject: [PATCH 03/15] Add changeset --- .changeset/afraid-mugs-lay.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/afraid-mugs-lay.md diff --git a/.changeset/afraid-mugs-lay.md b/.changeset/afraid-mugs-lay.md new file mode 100644 index 0000000000..c2a3671f47 --- /dev/null +++ b/.changeset/afraid-mugs-lay.md @@ -0,0 +1,5 @@ +--- +"@navikt/ds-react": patch +--- + +Button: Fix edge-case where setting "loading={true}" in a Modal caused the button to get 0 width and not show spinner From 61627b41a95f51a000796d35bbdb11580aeb441f Mon Sep 17 00:00:00 2001 From: Vegard Haugstvedt Date: Wed, 30 Oct 2024 16:23:58 +0100 Subject: [PATCH 04/15] Replace width calculation with just visually hiding label + icon and absolutely positioning the loading icon. --- @navikt/core/css/button.css | 8 ++++ @navikt/core/react/src/button/Button.tsx | 60 +++++++----------------- 2 files changed, 26 insertions(+), 42 deletions(-) diff --git a/@navikt/core/css/button.css b/@navikt/core/css/button.css index a3062ad2a0..edbf9ed20c 100644 --- a/@navikt/core/css/button.css +++ b/@navikt/core/css/button.css @@ -496,6 +496,10 @@ } /* Loader overrides */ +.navds-button .navds-loader { + position: absolute; +} + .navds-button .navds-loader .navds-loader__foreground { stroke: var(--ac-button-loader-stroke, currentColor); } @@ -505,6 +509,10 @@ stroke: var(--ac-button-primary-loader-stroke-bg, rgb(255 255 255 / 0.3)); } +.navds-button:has(.navds-loader) :is(.navds-button__icon, .navds-label) { + visibility: hidden; +} + @media (forced-colors: active) { .navds-button:not(.navds-button--loading):where(:disabled, .navds-button--disabled) { opacity: 1; diff --git a/@navikt/core/react/src/button/Button.tsx b/@navikt/core/react/src/button/Button.tsx index c0e2ef11d9..a389ff6684 100644 --- a/@navikt/core/react/src/button/Button.tsx +++ b/@navikt/core/react/src/button/Button.tsx @@ -1,10 +1,9 @@ import cl from "clsx"; -import React, { forwardRef, useRef, useState } from "react"; +import React, { forwardRef, useRef } from "react"; import { Loader } from "../loader"; import { Label } from "../typography"; import { omit } from "../util"; import { composeEventHandlers } from "../util/composeEventHandlers"; -import { useClientLayoutEffect } from "../util/hooks"; import { useMergeRefs } from "../util/hooks/useMergeRefs"; import { OverridableComponent } from "../util/types"; @@ -82,29 +81,14 @@ export const Button: OverridableComponent = ref, ) => { const buttonRef = useRef(null); - const [widthOverride, setWidthOverride] = useState(); const mergedRef = useMergeRefs(buttonRef, ref); - useClientLayoutEffect(() => { - if (loading && buttonRef?.current?.getBoundingClientRect()?.width) { - const requestID = window.requestAnimationFrame(() => { - setWidthOverride( - buttonRef?.current?.getBoundingClientRect()?.width, - ); - }); - return () => { - setWidthOverride(undefined); - cancelAnimationFrame(requestID); - }; - } - }, [loading, children, buttonRef?.current]); - const filterProps: React.ButtonHTMLAttributes = - disabled ?? widthOverride ? omit(rest, ["href"]) : rest; + disabled ?? loading ? omit(rest, ["href"]) : rest; const handleKeyUp = (e: React.KeyboardEvent) => { - if (e.key === " " && !disabled && !widthOverride) { + if (e.key === " " && !disabled && !loading) { e.currentTarget.click(); } }; @@ -121,33 +105,25 @@ export const Button: OverridableComponent = `navds-button--${variant}`, `navds-button--${size}`, { - "navds-button--loading": widthOverride, + "navds-button--loading": loading, "navds-button--icon-only": !!icon && !children, - "navds-button--disabled": disabled ?? widthOverride, + "navds-button--disabled": disabled ?? loading, }, )} - style={{ - ...style, - width: widthOverride, - }} - disabled={disabled ?? widthOverride ? true : undefined} + style={style} + disabled={disabled ?? loading ? true : undefined} > - {loading ? ( - - ) : ( - <> - {icon && iconPosition === "left" && ( - {icon} - )} - {children && ( - - )} - {icon && iconPosition === "right" && ( - {icon} - )} - + {icon && iconPosition === "left" && ( + {icon} + )} + {loading && } + {children && ( + + )} + {icon && iconPosition === "right" && ( + {icon} )} ); From d3b62423eaa0dcd809ae62bb9c0927a3b6bd53df Mon Sep 17 00:00:00 2001 From: Vegard Haugstvedt Date: Fri, 6 Dec 2024 12:22:07 +0100 Subject: [PATCH 05/15] Update @navikt/core/react/src/button/Button.tsx --- @navikt/core/react/src/button/Button.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/@navikt/core/react/src/button/Button.tsx b/@navikt/core/react/src/button/Button.tsx index a389ff6684..bd91e4b867 100644 --- a/@navikt/core/react/src/button/Button.tsx +++ b/@navikt/core/react/src/button/Button.tsx @@ -85,7 +85,7 @@ export const Button: OverridableComponent = const mergedRef = useMergeRefs(buttonRef, ref); const filterProps: React.ButtonHTMLAttributes = - disabled ?? loading ? omit(rest, ["href"]) : rest; + (disabled || loading) ? omit(rest, ["href"]) : rest; const handleKeyUp = (e: React.KeyboardEvent) => { if (e.key === " " && !disabled && !loading) { From 3a92850c73161b0622241714b8300c0a4014fda8 Mon Sep 17 00:00:00 2001 From: Julian Nymark Date: Thu, 19 Dec 2024 14:48:47 +0100 Subject: [PATCH 06/15] add story for bug case --- .../core/react/src/button/button.stories.tsx | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/@navikt/core/react/src/button/button.stories.tsx b/@navikt/core/react/src/button/button.stories.tsx index a38e01441b..becd8bf8f0 100644 --- a/@navikt/core/react/src/button/button.stories.tsx +++ b/@navikt/core/react/src/button/button.stories.tsx @@ -2,6 +2,8 @@ import { StoryObj } from "@storybook/react"; import React from "react"; import { StarIcon as BaseStarIcon } from "@navikt/aksel-icons"; import { HStack, VStack } from "../layout/stack"; +import { Modal } from "../modal"; +import { BodyLong } from "../typography"; import { Button } from "./index"; export default { @@ -154,6 +156,51 @@ export const DisabledAsLink: Story = { render: () => , }; +export const InsideModal: Story = { + render: () => { + const ref = React.useRef(null); + + return ( +
+ + + + + + Culpa aliquip ut cupidatat laborum minim quis ex in aliqua. Qui + incididunt dolor do ad ut. Incididunt eiusmod nostrud deserunt + duis laborum. Proident aute culpa qui nostrud velit adipisicing + minim. Consequat aliqua aute dolor do sit Lorem nisi mollit velit. + Aliqua exercitation non minim minim pariatur sunt laborum ipsum. + Exercitation nostrud est laborum magna non non aliqua qui esse. + + + + + + + + +
+ ); + }, +}; + export const Chromatic: Story = { render: () => ( From d50d18cceef865d569007eaf92bc3f180dba045d Mon Sep 17 00:00:00 2001 From: Vegard Haugstvedt Date: Thu, 19 Dec 2024 15:22:35 +0100 Subject: [PATCH 07/15] We no longer use buttonRef/mergedRef internally, so removed them and set ref directly --- @navikt/core/react/src/button/Button.tsx | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/@navikt/core/react/src/button/Button.tsx b/@navikt/core/react/src/button/Button.tsx index bd91e4b867..cf89b993ac 100644 --- a/@navikt/core/react/src/button/Button.tsx +++ b/@navikt/core/react/src/button/Button.tsx @@ -1,10 +1,9 @@ import cl from "clsx"; -import React, { forwardRef, useRef } from "react"; +import React, { forwardRef } from "react"; import { Loader } from "../loader"; import { Label } from "../typography"; import { omit } from "../util"; import { composeEventHandlers } from "../util/composeEventHandlers"; -import { useMergeRefs } from "../util/hooks/useMergeRefs"; import { OverridableComponent } from "../util/types"; export interface ButtonProps @@ -80,12 +79,8 @@ export const Button: OverridableComponent = }, ref, ) => { - const buttonRef = useRef(null); - - const mergedRef = useMergeRefs(buttonRef, ref); - const filterProps: React.ButtonHTMLAttributes = - (disabled || loading) ? omit(rest, ["href"]) : rest; + disabled || loading ? omit(rest, ["href"]) : rest; const handleKeyUp = (e: React.KeyboardEvent) => { if (e.key === " " && !disabled && !loading) { @@ -97,7 +92,7 @@ export const Button: OverridableComponent = Date: Thu, 19 Dec 2024 15:30:12 +0100 Subject: [PATCH 08/15] Update @navikt/core/css/button.css Co-authored-by: Ken <26967723+KenAJoh@users.noreply.github.com> --- @navikt/core/css/button.css | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/@navikt/core/css/button.css b/@navikt/core/css/button.css index 0ff76c6111..7e42e63f2d 100644 --- a/@navikt/core/css/button.css +++ b/@navikt/core/css/button.css @@ -510,7 +510,7 @@ stroke: var(--ac-button-primary-loader-stroke-bg, rgb(255 255 255 / 0.3)); } -.navds-button:has(.navds-loader) :is(.navds-button__icon, .navds-label) { +.navds-button--loading :is(.navds-button__icon, .navds-label) { visibility: hidden; } From 28ad7b62d69a4a835904eebe1e7f6634dc2c06fc Mon Sep 17 00:00:00 2001 From: Vegard Haugstvedt Date: Thu, 19 Dec 2024 15:34:51 +0100 Subject: [PATCH 09/15] Hiding everything except loading spinner is more robust than hiding every specific item. --- @navikt/core/css/button.css | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/@navikt/core/css/button.css b/@navikt/core/css/button.css index 7e42e63f2d..2e5bad48d1 100644 --- a/@navikt/core/css/button.css +++ b/@navikt/core/css/button.css @@ -510,7 +510,7 @@ stroke: var(--ac-button-primary-loader-stroke-bg, rgb(255 255 255 / 0.3)); } -.navds-button--loading :is(.navds-button__icon, .navds-label) { +.navds-button--loading > :is(:not(.navds-loader)) { visibility: hidden; } From ee1cfddd44ce702c3ae698ff0eea3d9fc6d18854 Mon Sep 17 00:00:00 2001 From: Vegard Haugstvedt Date: Thu, 19 Dec 2024 16:01:31 +0100 Subject: [PATCH 10/15] In loading-state, the button icon will no longer be able to be the only child (hidden), because of the loader icon. Instead checks for no label. Could consider :not(:has(:is(.navds-button__icon, .navds-loader))) as well, to be more robust, but it might be overkill. --- @navikt/core/css/button.css | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/@navikt/core/css/button.css b/@navikt/core/css/button.css index 2e5bad48d1..e2f860f8c3 100644 --- a/@navikt/core/css/button.css +++ b/@navikt/core/css/button.css @@ -109,7 +109,7 @@ margin-right: var(--ac-button-icon-margin); } -.navds-button__icon:only-child { +.navds-button:not(:has(.navds-label)) .navds-button__icon { margin: 0; } From d0015075e73036ac722ddec02023c2486594b5d1 Mon Sep 17 00:00:00 2001 From: Vegard Haugstvedt Date: Thu, 19 Dec 2024 16:24:23 +0100 Subject: [PATCH 11/15] Much simpler/easier to read --- @navikt/core/css/button.css | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/@navikt/core/css/button.css b/@navikt/core/css/button.css index e2f860f8c3..68e1f27472 100644 --- a/@navikt/core/css/button.css +++ b/@navikt/core/css/button.css @@ -109,7 +109,7 @@ margin-right: var(--ac-button-icon-margin); } -.navds-button:not(:has(.navds-label)) .navds-button__icon { +.navds-button--icon-only .navds-button__icon { margin: 0; } From 6a8b397d4026a7fa5e94e351432dda1e2b3a82a1 Mon Sep 17 00:00:00 2001 From: Vegard Haugstvedt Date: Thu, 19 Dec 2024 16:28:54 +0100 Subject: [PATCH 12/15] Don't need to destructure style, only to add it along with rest. --- @navikt/core/react/src/button/Button.tsx | 2 -- 1 file changed, 2 deletions(-) diff --git a/@navikt/core/react/src/button/Button.tsx b/@navikt/core/react/src/button/Button.tsx index cf89b993ac..4f8ff6de4d 100644 --- a/@navikt/core/react/src/button/Button.tsx +++ b/@navikt/core/react/src/button/Button.tsx @@ -72,7 +72,6 @@ export const Button: OverridableComponent = size = "medium", loading = false, disabled, - style, icon, iconPosition = "left", ...rest @@ -105,7 +104,6 @@ export const Button: OverridableComponent = "navds-button--disabled": disabled ?? loading, }, )} - style={style} disabled={disabled ?? loading ? true : undefined} > {icon && iconPosition === "left" && ( From cee50266fef90b0a910b3b0dccf8a39f86b2d56a Mon Sep 17 00:00:00 2001 From: Vegard Haugstvedt Date: Thu, 19 Dec 2024 16:35:15 +0100 Subject: [PATCH 13/15] Should destructure onKeyUp from props, instead of having it added through filteredProps and then overridden afterwards. --- @navikt/core/react/src/button/Button.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/@navikt/core/react/src/button/Button.tsx b/@navikt/core/react/src/button/Button.tsx index 4f8ff6de4d..4811a2f15a 100644 --- a/@navikt/core/react/src/button/Button.tsx +++ b/@navikt/core/react/src/button/Button.tsx @@ -74,6 +74,7 @@ export const Button: OverridableComponent = disabled, icon, iconPosition = "left", + onKeyUp, ...rest }, ref, @@ -92,7 +93,7 @@ export const Button: OverridableComponent = {...(Component !== "button" ? { role: "button" } : {})} {...filterProps} ref={ref} - onKeyUp={composeEventHandlers(filterProps.onKeyUp, handleKeyUp)} + onKeyUp={composeEventHandlers(onKeyUp, handleKeyUp)} className={cl( className, "navds-button", From d57ea6a1167cd79ea1207d789eeb81b3a8dcee04 Mon Sep 17 00:00:00 2001 From: Vegard Haugstvedt Date: Thu, 19 Dec 2024 17:21:14 +0100 Subject: [PATCH 14/15] Quick and easy conversion for darkside --- @navikt/core/css/darkside/button.darkside.css | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/@navikt/core/css/darkside/button.darkside.css b/@navikt/core/css/darkside/button.darkside.css index bd292d82df..c1013ad27d 100644 --- a/@navikt/core/css/darkside/button.darkside.css +++ b/@navikt/core/css/darkside/button.darkside.css @@ -69,7 +69,7 @@ margin-right: var(--__axc-button-icon-margin); } - &:only-child { + .navds-button--icon-only & { margin: 0; } } @@ -264,6 +264,10 @@ } /* Loader overrides */ +.navds-button .navds-loader { + position: absolute; +} + .navds-button .navds-loader .navds-loader__foreground { stroke: currentColor; } @@ -273,6 +277,10 @@ stroke: rgb(255 255 255 / 0.3); } +.navds-button--loading > :is(:not(.navds-loader)) { + visibility: hidden; +} + @media (forced-colors: active) { .navds-button:not(.navds-button--loading):where(:disabled, .navds-button--disabled) { opacity: 1; From 0350ef5808591a9ff6127d0202cec14712ffa737 Mon Sep 17 00:00:00 2001 From: Vegard Haugstvedt Date: Fri, 20 Dec 2024 09:52:07 +0100 Subject: [PATCH 15/15] Unnecessary :is after refactoring --- @navikt/core/css/button.css | 2 +- @navikt/core/css/darkside/button.darkside.css | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/@navikt/core/css/button.css b/@navikt/core/css/button.css index 68e1f27472..0f2e66779c 100644 --- a/@navikt/core/css/button.css +++ b/@navikt/core/css/button.css @@ -510,7 +510,7 @@ stroke: var(--ac-button-primary-loader-stroke-bg, rgb(255 255 255 / 0.3)); } -.navds-button--loading > :is(:not(.navds-loader)) { +.navds-button--loading > :not(.navds-loader) { visibility: hidden; } diff --git a/@navikt/core/css/darkside/button.darkside.css b/@navikt/core/css/darkside/button.darkside.css index c1013ad27d..9021dbb055 100644 --- a/@navikt/core/css/darkside/button.darkside.css +++ b/@navikt/core/css/darkside/button.darkside.css @@ -277,7 +277,7 @@ stroke: rgb(255 255 255 / 0.3); } -.navds-button--loading > :is(:not(.navds-loader)) { +.navds-button--loading > :not(.navds-loader) { visibility: hidden; }