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

ButtonGroup-konsept #4504

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

ButtonGroup-konsept #4504

wants to merge 2 commits into from

Conversation

piofinn
Copy link
Contributor

@piofinn piofinn commented Jan 27, 2025

En tenkt implementasjon av ButtonGroup fra issue #4503 .

@piofinn piofinn changed the title 4503 button group ButtonGroup-konsept Jan 27, 2025
@fremtind-bot
Copy link
Collaborator

fremtind-bot commented Jan 27, 2025

Forhåndsvisning: https://jokul.fremtind.no/preview/4503-button-group/
🔍 Commit: 756fda9

Forhåndsvisningen blir tilgjengelig innen et par minutter. Den fjernes automatisk når pull requesten lukkes.

fremtind-bot added a commit that referenced this pull request Jan 27, 2025
@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.

// 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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants