Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix/button loading issue in modal #3252

Merged
merged 20 commits into from
Dec 20, 2024
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
2485ff3
Check loading prop for showing spinner, not widthOverride.
it-vegard Oct 18, 2024
e09bab9
Prevent setting width to 0 while showing spinner, to avoid issue in M…
it-vegard Oct 18, 2024
1d500d7
Add changeset
it-vegard Oct 18, 2024
61627b4
Replace width calculation with just visually hiding label + icon and …
it-vegard Oct 30, 2024
af84c4c
Merge branch 'main' into bugfix/button-loading-issue-in-modal
it-vegard Oct 30, 2024
d3b6242
Update @navikt/core/react/src/button/Button.tsx
it-vegard Dec 6, 2024
3a92850
add story for bug case
JulianNymark Dec 19, 2024
9604a7b
Merge branch 'main' into bugfix/button-loading-issue-in-modal
JulianNymark Dec 19, 2024
d50d18c
We no longer use buttonRef/mergedRef internally, so removed them and …
it-vegard Dec 19, 2024
8cc3915
Update @navikt/core/css/button.css
it-vegard Dec 19, 2024
28ad7b6
Hiding everything except loading spinner is more robust than hiding e…
it-vegard Dec 19, 2024
ee1cfdd
In loading-state, the button icon will no longer be able to be the on…
it-vegard Dec 19, 2024
d001507
Much simpler/easier to read
it-vegard Dec 19, 2024
6a8b397
Don't need to destructure style, only to add it along with rest.
it-vegard Dec 19, 2024
cee5026
Should destructure onKeyUp from props, instead of having it added thr…
it-vegard Dec 19, 2024
1e0f841
Merge branch 'main' into bugfix/button-loading-issue-in-modal
JulianNymark Dec 19, 2024
d57ea6a
Quick and easy conversion for darkside
it-vegard Dec 19, 2024
2876f26
Merge branch 'main' into bugfix/button-loading-issue-in-modal
it-vegard Dec 19, 2024
0350ef5
Unnecessary :is after refactoring
it-vegard Dec 20, 2024
ce25ccd
Merge branch 'main' into bugfix/button-loading-issue-in-modal
it-vegard Dec 20, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/afraid-mugs-lay.md
Original file line number Diff line number Diff line change
@@ -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
10 changes: 9 additions & 1 deletion @navikt/core/css/button.css
it-vegard marked this conversation as resolved.
Show resolved Hide resolved
it-vegard marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now the only thing left is to update button.darkside.css as well 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was about to comment that. 🙈

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a quick edit for darkside. I'll welcome suggested improvements, e.g. for nesting choices.

Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@
margin-right: var(--ac-button-icon-margin);
}

.navds-button__icon:only-child {
.navds-button--icon-only .navds-button__icon {
margin: 0;
}

Expand Down Expand Up @@ -497,6 +497,10 @@
}

/* Loader overrides */
.navds-button .navds-loader {
position: absolute;
}

.navds-button .navds-loader .navds-loader__foreground {
stroke: var(--ac-button-loader-stroke, currentColor);
}
Expand All @@ -506,6 +510,10 @@
stroke: var(--ac-button-primary-loader-stroke-bg, 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;
Expand Down
70 changes: 20 additions & 50 deletions @navikt/core/react/src/button/Button.tsx
it-vegard marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
import cl from "clsx";
import React, { forwardRef, useRef, useState } 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 { useClientLayoutEffect } from "../util/hooks";
import { useMergeRefs } from "../util/hooks/useMergeRefs";
import { OverridableComponent } from "../util/types";

export interface ButtonProps
Expand Down Expand Up @@ -74,37 +72,18 @@ export const Button: OverridableComponent<ButtonProps, HTMLButtonElement> =
size = "medium",
loading = false,
disabled,
style,
icon,
iconPosition = "left",
onKeyUp,
...rest
},
ref,
) => {
const buttonRef = useRef<HTMLButtonElement | null>(null);
const [widthOverride, setWidthOverride] = useState<number>();

const mergedRef = useMergeRefs(buttonRef, ref);

useClientLayoutEffect(() => {
if (loading) {
const requestID = window.requestAnimationFrame(() => {
setWidthOverride(
buttonRef?.current?.getBoundingClientRect()?.width,
);
});
return () => {
setWidthOverride(undefined);
cancelAnimationFrame(requestID);
};
}
}, [loading, children]);

const filterProps: React.ButtonHTMLAttributes<HTMLButtonElement> =
disabled ?? widthOverride ? omit(rest, ["href"]) : rest;
disabled || loading ? omit(rest, ["href"]) : rest;

const handleKeyUp = (e: React.KeyboardEvent<HTMLButtonElement>) => {
if (e.key === " " && !disabled && !widthOverride) {
if (e.key === " " && !disabled && !loading) {
e.currentTarget.click();
}
};
Expand All @@ -113,41 +92,32 @@ export const Button: OverridableComponent<ButtonProps, HTMLButtonElement> =
<Component
{...(Component !== "button" ? { role: "button" } : {})}
{...filterProps}
ref={mergedRef}
onKeyUp={composeEventHandlers(filterProps.onKeyUp, handleKeyUp)}
ref={ref}
onKeyUp={composeEventHandlers(onKeyUp, handleKeyUp)}
className={cl(
className,
"navds-button",
`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}
disabled={disabled ?? loading ? true : undefined}
>
{widthOverride ? (
<Loader size={size} />
) : (
<>
{icon && iconPosition === "left" && (
<span className="navds-button__icon">{icon}</span>
)}
{children && (
<Label as="span" size={size === "medium" ? "medium" : "small"}>
{children}
</Label>
)}
{icon && iconPosition === "right" && (
<span className="navds-button__icon">{icon}</span>
)}
</>
{icon && iconPosition === "left" && (
<span className="navds-button__icon">{icon}</span>
)}
{loading && <Loader size={size} />}
{children && (
<Label as="span" size={size === "medium" ? "medium" : "small"}>
{children}
</Label>
)}
{icon && iconPosition === "right" && (
<span className="navds-button__icon">{icon}</span>
)}
</Component>
);
Expand Down
47 changes: 47 additions & 0 deletions @navikt/core/react/src/button/button.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -154,6 +156,51 @@ export const DisabledAsLink: Story = {
render: () => <ButtonGrid disabled href="#" as="a" />,
};

export const InsideModal: Story = {
render: () => {
const ref = React.useRef<HTMLDialogElement>(null);

return (
<div className="py-16">
<Button onClick={() => ref.current?.showModal()}>Åpne modal</Button>

<Modal ref={ref} header={{ heading: "Overskrift" }}>
<Modal.Body>
<BodyLong>
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.
</BodyLong>
</Modal.Body>
<Modal.Footer>
<Button loading type="button" onClick={() => ref.current?.close()}>
Primær
</Button>
<Button
loading
type="button"
variant="secondary"
onClick={() => ref.current?.close()}
>
Sekundær
</Button>
<Button
type="button"
variant="tertiary"
onClick={() => ref.current?.close()}
>
Tertiær
</Button>
</Modal.Footer>
</Modal>
</div>
);
},
};

export const Chromatic: Story = {
render: () => (
<VStack gap="6" align="center">
Expand Down
Loading