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.

Sign up for GitHub

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

ButtonGroup-konsept #4504

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

ButtonGroup-konsept #4504

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e689fb0
feat: legg til komponent for gruppering av knapper, ButtonGroup
piofinn Jan 27, 2025
6780482
feat: ta i bruk ButtonGroup for handlinger i Modal
piofinn Jan 27, 2025
963f575
feat: legg til mulighet for å tvinge vertikal layout i ButtonGroup
piofinn Jan 29, 2025
2f4ca23
refactor: bruk Flex-komponenten for layout i ButtonGroup
piofinn Feb 3, 2025
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
13 changes: 13 additions & 0 deletions packages/jokul/src/components/button/ButtonGroup.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { clsx } from "clsx";
import type { ComponentPropsWithRef } from "react";

// TODO: Denne burde kanskje være polymorf, og i hvert fall types bedre
export const ButtonGroup = ({
className,
children,
...props
}: ComponentPropsWithRef<"div">): JSX.Element => (
<div className={clsx("jkl-button-group", className)} {...props}>

Check failure on line 10 in packages/jokul/src/components/button/ButtonGroup.tsx

View workflow job for this annotation

GitHub Actions / Kjør tester og linting 

'React' refers to a UMD global, but the current file is a module. Consider adding an import instead.
{children}
</div>
);
3 changes: 2 additions & 1 deletion packages/jokul/src/components/button/styles/_index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
@forward "button";
@forward "button-group";

@use "../../loader/styles" as button;
@use "../../loader/styles" as loader;
10 changes: 10 additions & 0 deletions packages/jokul/src/components/button/styles/button-group.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
@use "../../../core/jkl";

.jkl-button-group {
container: --button-group / inline-size;
Copy link
Contributor

Choose a reason for hiding this comment

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

Min første reaksjon her var er dette en CSS variabel?

Mulig det er min feil fordi jeg ikke har sett så mye på syntaxen til container-queries enda men jeg tror jeg ville foretrukket å droppe -- prefix for container-name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hehe, ser absolutt hvor reaksjonen kommer fra! Dette er en dashed-ident, som for så vidt er det samme som brukes til å navngi custom variabler. Det er i ferd med å bli standard for å navngi egendefinerte verdier i CSS.

container-name støtter også "vanlig" custom-ident, så vi kan fint kjøre uten -- her! Tenkte bare jeg skulle være litt foran kurven 😅 For eksempel støtter anchor() API-et for plassering kun dashed-ident, så man må nok bli vant til det før eller senere uansett.


width: 100%;
display: flex;
gap: jkl.$unit-15 jkl.$unit-20;
flex-wrap: wrap;
}
15 changes: 9 additions & 6 deletions packages/jokul/src/components/button/styles/button.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,11 @@ $_flash-animation-name: jkl-tertiary-flash-#{string.unique-id()};
overflow: hidden;
max-width: 100%;

// La knappene ta full bredde hvis de er i en smal ButtonGroup
@container --button-group (max-width: 350px) {
width: 100%;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Dette vil utvilsomt se bedre ut i CookieConsent. Jeg var faktisk litt på gjerdet på om jeg skulle bruke jkl.screen-from i stedet for jkl.from-medium-device fordi jeg syns knapper i full bredde øverst i "small device" kategorien vår blir litt vel brede.

Copy link
Contributor

Choose a reason for hiding this comment

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

Kanskje 350px her kunne kommet fra en CSS-variabel med en default slik at konsumenten av komponenten kan overstyre om de har veldig lange knappetekster.

    @container --button-group (max-width: var(--jkl-button-group-breakpoint, 350px)) {
        width: 100%;
    }

Eller noe slikt, gitt at det er lov å bruke variabler der da.

Copy link
Contributor Author

@piofinn piofinn Jan 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

En variabel er nok en god idé for en ordentlig implementasjon av dette, ja!
Edit: Det funker visst ikke med custom variables i media- og container queries.


@include jkl.motion("standard", "productive", scale);

&:has(.jkl-icon:first-child) {
Expand Down Expand Up @@ -196,15 +201,13 @@ $_flash-animation-name: jkl-tertiary-flash-#{string.unique-id()};
}

&:has(.jkl-icon:first-child) {
padding-inline: var(--jkl-button-teritary-padding-icon) calc(
var(--jkl-button-teritary-padding-icon) * 2
);
padding-inline: var(--jkl-button-teritary-padding-icon)
calc(var(--jkl-button-teritary-padding-icon) * 2);
}

&:has(.jkl-icon:last-child) {
padding-inline: calc(
var(--jkl-button-teritary-padding-icon) * 2
) var(--jkl-button-teritary-padding-icon);
padding-inline: calc(var(--jkl-button-teritary-padding-icon) * 2)
var(--jkl-button-teritary-padding-icon);
}
}

Expand Down
3 changes: 2 additions & 1 deletion packages/jokul/src/components/modal/Modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import clsx from "clsx";
import React, { forwardRef } from "react";
import { WithOptionalChildren } from "../../core/types.js";
import { ButtonGroup } from "../button/ButtonGroup.js";
import { CloseIcon } from "../icon/icons/CloseIcon.js";
import { IconButton, IconButtonProps } from "../icon-button/IconButton.js";
import { ModalConfig } from "./useModal.js";
Expand Down Expand Up @@ -150,7 +151,7 @@ ModalBody.displayName = "ModalBody";
*/
export const ModalActions = forwardRef<HTMLDivElement, BaseModalProps>(
({ className, ...rest }, ref) => (
<div
<ButtonGroup
className={clsx("jkl-modal-actions", className)}
{...rest}
ref={ref}
Expand Down
3 changes: 0 additions & 3 deletions packages/jokul/src/components/modal/styles/modal.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,4 @@

.jkl-modal-actions {
margin: var(--jkl-modal-actions-margin);
display: flex;
flex-direction: row;
gap: jkl.$unit-20;
}
Loading