feat(nve-layout): layoutkomponenter#907
Conversation
|
Du har sikkert tenkt på dette, men burde det heller hete nve-box, nve-layout, nve-stack osv. for å lettere se i koden at det kommer fra nve designsystemet og ikke noe "egendefinert"? Er usikker på hva som er best, kanskje flere har noen meninger på det? |
Jeg lurte på det samme! Ser også at f.eks. aksel.nav.no bruker bare "box" og "stack", så jeg er usikker på hva som er best her |
gruble
left a comment
There was a problem hiding this comment.
Bra jobba med dokumentsjon og gode eksempler.
Jeg liker at komponentene er basert på tanker og problemstillinger som er kjent og godt dokumentert fra før (på https://every-layout.dev/).
Vi får noen pedagogiske utfordringer fordi vi bruker size for å sette gap og padding. Likevel tilbyr vi overstyring ved å sette gap, padding direkte. Dette synes jeg er litt forvirrende. Kan vi ende opp med at noen angir gap eller padding med token-verdier i stedet for å bruke size og er dette egentlig et problem?
Jeg må innrømme at jeg falt av ganske fort da jeg forsøkte å lese de eksemplene som var tilgjengelige på https://every-layout.dev/
Er det virkelig så komplisert å lage god responsiv layout?
Jeg driver fortsatt og lærer meg CSS og det er mye jeg ikke skjønner.
Men jeg har stort sett greid å få til den layouten jeg ønsker med å bruke CSS likevel.
Trenger vi da et ekstra lag oppå CSS for å håndtere layout?
Hvis vi velger å ta i bruk egne komponenter for layout i designsystemet, ønsker jeg at vi dokumenetrer fordeler og ulemper med dette i https://github.com/NVE/Designsystem/blob/main/beslutninger.md og hvorfor vi bestemmer oss for å bruke dem. Kanskje bør vi skrive noe her uansett om vi går for det eller ikke, så slipper vi omkamper:)
|
|
||
| `box-layout` pakker innhold i en boks med konsistent padding. | ||
|
|
||
| I de aller fleste tilfeller skal du bruke `size`. `size` er knyttet direkte til spacing-tokenene i designsystemet og sikrer at paddingen er konsistent på tvers av sider og komponenter. |
There was a problem hiding this comment.
Jeg sliter litt med å forstå dette konseptet.
Stemmer det at size kun setter padding og ikke påvirker selve størrelsen på boksen?
Om det stemmer, hadde det vært supert om vi kunne skrive dette enda mer tydelig, enten i dette avsnittet eller i avsnittet for stærrelse.
There was a problem hiding this comment.
Mange gode poeng her, @gruble!
Hovedpoenget med disse layoutene er for å simplifisere CSS og layout for de som ønsker det. Dette blir selvfølgelig valgfritt, og ønsker man å fortsette å bruke egen CSS for å definere dette er det absolutt mulig! Tanken er bare at man skal kunne bygge opp en responsiv layout raskt, slik at fokus kan være på logikk og utseende heller enn responsivitet 😊
Jeg er enig i at det er forvirrende med både gap og size, og kanskje jeg bare kan fjerne gap? Da kan man velge å bruke spacing-tokens rett fra DS i layout-komponentene, og om disse ikke passer, så kan man lage sin egen CSS som vanlig. Tror kanskje det vil hjelpe å gjøre det mindre forvirrende 😅 Vi må jo ikke ta alle prinsippene fra Every Layout.
Jeg kan legge til et avsnitt i beslutninger-filen så dette er dokumentert!
There was a problem hiding this comment.
Flott.
Er det mulig å bytte navn på size til gap? Jeg synes gap-begrepet passer bedre med hva dette brukes til.
Aner ikke hvordan man da kan løse å bruke egne tokens for gap, men da har man jo alltids mulighet til å bruke css i stedet for layout-komponenten, hvis man trenger mer kontroll.
There was a problem hiding this comment.
i boks skal vi ikke ha gap @gruble. boksen setter kun layout "rundt" komponenter så her blir det bare padding og evt ramme. gap skal du ha i stack f.eks.
|
Veldig kult at dere ser på slike komponenter, og bra docs og oppsett ! Jeg har ikke sett så nøye på alt men legger bare inn noen punkter her jeg kommer på etter en rask titt:
|
Det er fordi aksel lager ikke web komponenter, men react komponenter så de trenger ikke å bruke prefixer :). Vi bruker prefixer på alle komponentene våre, så ja, enig med Lisa. Vi bør ha nve- prefix på alle disse her ellers må man skrive en ekstra konfig i vue eller react applikasjoner slik at de skjønner hva "box-layout"-html element betyr. I dag sørger vi det det fungerer på alle nve- komponenter med (eksempel fra vue): export default defineConfig({
plugins: [
vue({
template: {
compilerOptions: {
isCustomElement: (tag) => tag.includes('nve-'),
},
},
}),
],
}); |
| import { LitElement, html } from 'lit'; | ||
|
|
||
| /** | ||
| * Stabler barn-elementer vertikalt med konsistent mellomrom. |
There was a problem hiding this comment.
Jeg stemmer for å slå sammen Stack og Cluster. Den eneste forskjellen mellom dem virker å være retningen, så vi kunne hatt én komponent med en orientation-property som styrer om elementene legges vertikalt eller horisontalt. Færre komponenter er enklere å vedlikeholde tenker jeg ;)
There was a problem hiding this comment.
Jeg tror det kan være hensiktsmessig å beholde disse som to forskjellige komponenter. Noe av fordelen med å ha de separat er at vi får mindre komponenter som gjør spesifikke ting i stedet for komponenter som gjør mye og fort kan bli bloated hvis behov endrer seg.
Jeg tror også at det semantiske er noe vi må se på. En Cluster forteller utvikleren med en gang at noe skal legges horisontalt, samme med Stack og vertikalt. Har vi bare en Stack med orientation-props er det ikke like intuitivt for utvikleren å se hva som skjer, og vi åpner for mange flere kombinasjoner av props, noe som kan føre til at det blir inkonsistent bruk på tvers av prosjektene.
Har dere noen tanker om dette, @lisamarimyreneNVE, @gruble, @olmabo?
There was a problem hiding this comment.
Siden det allerede er etablerte pattern på de forskjellige, gir det kanskje mest mening å ha de separat for å følge en mest mulig standard.
There was a problem hiding this comment.
Jeg ser poenget ditt, men jeg er ikke enig fortsatt (sorry).
For meg betyr Cluster at flere ting er gruppert eller samlet på ett sted. Jeg ville aldri automatisk tolket at "Cluster" betyr et horisontalt layout. Derimot er Stack et mønster som brukes i mange store komponentbiblioteker, hvor orientering (vertikal eller horisontal) styres med en orientation- eller direction-prop.
Slik jeg ser det, er det kun flex-orientation som skiller disse to komponentene. Det føles derfor unødvendig å vedlikeholde to nesten identiske komponenter når forskjellen er så liten. Med én komponent blir det mindre kode å vedlikeholde og færre steder som må oppdateres dersom vi ønsker å endre funksjonalitet eller API senere.
Vil gjerne høre fra andre og.
There was a problem hiding this comment.
Jeg ser at aksel bruker HStack og VStack. Syns kanskje det virker mer intuitivt enn Cluster og Stack når vi snakker om retningen.
There was a problem hiding this comment.
En annen ting er at vi allerede bruker begrepet Cluster i kartløsningene våre, hvor flere markører eller punkter slås sammen til én representasjon. Der beskriver navnet veldig godt hva komponenten gjør – den grupperer elementer og viser brukeren at det finnes flere objekter under én markør (dette er en global standard for å kalle den slik).
Derfor synes jeg det kan bli litt navn konflikter her. Det kan fort skape misforståelser både i kodebasen og for utviklere som er nye i prosjektet.
Av den grunn mener jeg at en generell Stack med en orientation-prop er et mer naturlig og gjenkjennelig API, samtidig som vi unngår navnekonflikter med kartkomponentene våre. Evt StackHorizotnal eller noe lignende som Aksel gjor.
There was a problem hiding this comment.
Jeg er helt enig i at Cluster betyr at ting er gruppert, og det er på en måte akkurat det jeg vil frem til! Cluster (i layout-sammenheng) betyr at elementer er gruppert og oppfører seg på en klyngemåte med wrapping. Derfor synes jeg navnet passer godt ettersom det beskriver akkurat det komponenten gjør. Cluster er også et veletablert begrep som baserer seg på Every Layout.
Teknisk sett er forskjellen liten, men konseptene er veldig forskjellige. En Stack representerer en lineær layout, mens Cluster handler om gruppering og wrapping. Derfor synes jeg ikke HStack og VStack gir like mye mening konseptuelt.
Når det gjelder navnet Cluster så skjønner jeg hva du sier om at vi allerede bruker det i kartløsningene våre. Samtidig beskriver begge tilfellene egentlig den samme tanken om at elementer grupperes og presenteres som en enhet. Derfor ser jeg på det mer som konsistent begrepsbruk heller enn en navnekonflikt. Siden kart-clustering og layout-clustering brukes i ulike kontekster tror jeg ikke det vil skape forvirring.
Men jeg er veldig åpen for flere meninger om dette! @gruble, @olmabo?
|
|
||
| `box-layout` pakker innhold i en boks med konsistent padding. | ||
|
|
||
| I de aller fleste tilfeller skal du bruke `size`. `size` er knyttet direkte til spacing-tokenene i designsystemet og sikrer at paddingen er konsistent på tvers av sider og komponenter. |
There was a problem hiding this comment.
i boks skal vi ikke ha gap @gruble. boksen setter kun layout "rundt" komponenter så her blir det bare padding og evt ramme. gap skal du ha i stack f.eks.
Ja det er mulig det :) |
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://brave-meadow-0c645bd03-907.westeurope.5.azurestaticapps.net |
| @@ -0,0 +1,56 @@ | |||
| import { customElement, property } from 'lit/decorators.js'; | |||
There was a problem hiding this comment.
Siden alle layout-komponentene importeres fra layout-mappen, bør vi skrive i dokumentasjonen hvordan de importeres fra nve-designsystem i applikasjoner som bruker dem.
Kunne du lagt til et avsnitt om import i dokumentasjonen for hver av layout-komponentene?
|
|
||
| ## Padding | ||
|
|
||
| `padding` setter padding rundt innholdet ved bruk av et spacing-token. Hvis `padding` ikke er satt, brukes `medium` som standard. |
There was a problem hiding this comment.
jeg tror vi kanskje bør skrive litt mer om hvilke verdier som faktisk kan brukes her. det står bare medium, men hvilke verdier er gyldige? jeg tror kanskje alle alternativene bør listes opp.
| */ | ||
| export class NveLayoutBase extends LitElement { | ||
| /** Tokenbasert padding på alle sider. */ | ||
| @property({ type: String, reflect: true }) padding?: SpacingToken; |
There was a problem hiding this comment.
det som fortsatt er et problem her, er at vi ikke håndterer fallbacks når folk skriver noe feil i parameterne.
jeg pratet litt med ChatGPT, og jeg er enig i at vi kanskje bør bruke default-verdi når brukeren skriver en ugyldig property-verdi. hvis det skjer, bør vi kanskje også bruke console.warn for å si fra om at verdien ikke er gyldig.
jeg har prøvd å skrive en verdi som ikke er gyldig, og da fikk jeg verken gap eller padding. det er kanskje ikke riktig. hva synes dere?
| @@ -0,0 +1,108 @@ | |||
| # nve-box | |||
There was a problem hiding this comment.
en ting jeg savner på siden til hver layout-komponent, er at det står at den importeres fra layout-mappen. det kan hende folk blir forvirret hvis de prøver å hente en layout-komponent fra nve-designsystem/components/nve-stack osv.
Layoutkomponenter:
stack,box,clusteroggridVi har til nå manglet layout i designsystemet, og endt opp med egendefinert css for layouts i hvert enkelt prosjekt. Denne PRen introduserer fire layout-primitiver som dekker det meste av hverdagsbehov. Layout-primitivene er basert på Every-Layout: https://every-layout.dev/
Hva er med?
stack-layout– stabler innhold vertikalt med mellomrom.box-layout– pakker inn innhold med padding og valgfri bakgrunnsfarge.cluster-layout– grupperer elementer horisontalt med automatisk linjebryting.grid-layout– responsivt rutenett som tilpasser antall kolonner etter tilgjengelig plass, helt uten media query.Alle komponentene bruker samme mønster: en
size-prop som er låst til spacing-tokenene våre og en separat gap/padding-prop for rå CSS-verdier i unntakstilfeller.Hva er ikke med?
Det ble vurdert å ta med: center-layout, columnist-layout, cover-layout, sidebar-layout og wrapper-layout, men jeg landet på at det er bedre å legge til når vi ser behovet kommer. Jeg tror det meste av bruk er dekket godt med de som tilbys nå.
Si gjerne ifra om du vil endre på noe!
fixes #898