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 5 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
8 changes: 8 additions & 0 deletions @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 @@ -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:has(.navds-loader) :is(.navds-button__icon, .navds-label) {
it-vegard marked this conversation as resolved.
Show resolved Hide resolved
visibility: hidden;
}

@media (forced-colors: active) {
.navds-button:not(.navds-button--loading):where(:disabled, .navds-button--disabled) {
opacity: 1;
Expand Down
60 changes: 18 additions & 42 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,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";

Expand Down Expand Up @@ -82,29 +81,14 @@ export const Button: OverridableComponent<ButtonProps, HTMLButtonElement> =
ref,
) => {
const buttonRef = useRef<HTMLButtonElement | null>(null);
it-vegard marked this conversation as resolved.
Show resolved Hide resolved
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;
it-vegard marked this conversation as resolved.
Show resolved Hide resolved

const handleKeyUp = (e: React.KeyboardEvent<HTMLButtonElement>) => {
if (e.key === " " && !disabled && !widthOverride) {
if (e.key === " " && !disabled && !loading) {
e.currentTarget.click();
}
};
Expand All @@ -121,33 +105,25 @@ export const Button: OverridableComponent<ButtonProps, HTMLButtonElement> =
`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}
it-vegard marked this conversation as resolved.
Show resolved Hide resolved
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
Loading