test: Skal ikke merges, men brukes for å lage en rask test og demonstrasjon av heading-komponent#802
test: Skal ikke merges, men brukes for å lage en rask test og demonstrasjon av heading-komponent#802tomapedersen wants to merge 3 commits intomainfrom
Conversation
…rasjon av heading-komponent
tomapedersen
requested review from
NVEJoel,
amish1188,
gruble,
lisamarimyreneNVE and
malingranlynve
as code owners
|
(!) Ikke merge denne. Den er kun for å teste og få tilbakemeldinger på enkle layout-komponenter |
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://brave-meadow-0c645bd03-802.westeurope.5.azurestaticapps.net |
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://brave-meadow-0c645bd03-802.westeurope.5.azurestaticapps.net |
| @property({ type: Number, reflect: true }) level: 1 | 2 | 3 | 4 | 5 | 6 = 1; | ||
|
|
||
| /** Typografitype - Kan overstyre det som er standard typografi basert på nivå */ | ||
| @property({ type: String, reflect: true }) typographyType?: |
There was a problem hiding this comment.
Nå vet jo alle hva jeg mener om å innføre komponenter for tekst i designsystemet, men hvis vi ender opp med å få dette, stemmer jeg for å ikke bruke "typography" i begreper i koden. Jeg mener det er unødvendig komplisert og vanskelig å stave riktig. Hva med å kalle det size eller type?
Verdien trenger heller kanskje ikke hete nøyaktig det samme som css-variabelen? Jeg skjønner vi trenger å prefikse css-variablene med "heading" , men i denne komponenten blir det "smør på flesk" å skrive:
<nve-heading level="2" typographyType="headingLarge"/>Overskrift</nve-heading>
Dette synes jeg ser bedre ut:
<nve-heading level="2" size="large">Overskrift</nve-heading>
Men denne synes jeg er best:
<h2>Overskrift</h2>
There was a problem hiding this comment.
Stemmer for kortere property-navn, men jeg synes ikke size er riktig her, siden vi peker til en token som inneholder font-size, font-weight, line-height og font-family. Size dekker ikke helt det vi faktisk styrer. Jeg tror enten variant eller type passer bedre her.
There was a problem hiding this comment.
ja, første jeg tenkte på var å bare bruke "xlarge" | "large" ... istedenfor. Men kopierte litt props fra React-komponenten i første omgang bare for å få noe konkret kode. Ville også ha inn en nve-komponent for å teste med for å verifisere at Accessibility-treet ble korrekt og at den takler role=heading elementer som ligger i shadow-dom (det ser ut til å virke ok)
Tror at size vil fungere som attributt, siden det meste annet er felles (alle har samme font og vekt, tror alle ha samme line-height i prosent)
There was a problem hiding this comment.
de bruker forskjellig line-height. jeg synes size hadde gitt mening hvis du satte opp en CSS-klasse som kun styrer font-size. Men siden det er font som endres her, blir size misvisende, tenker jeg.
There was a problem hiding this comment.
Ja, jeg regnet feil på line-height. Ser ut til at det korrekte er at alle har en utregnet line height på "font-size + ~6px"
xlarge har 40 / 46
large har 32 / 38.4
medium har 24 / 30
small har 20 / 26
xsmall har 18 / 23.4
Men absolutt verdt å diskutere noe annet enn size som navn
There was a problem hiding this comment.
Mulig at de ulike størrelsene også har ulik standard topp/bunn-marg, det har jeg ikke sjekket. Bør nok få avklart det med design og legge dem inn i css'en også
There was a problem hiding this comment.
vi kan diskutere det, ja. når jeg tenker litt mer over det, indikerer selve token-navnet også størrelsen, så kanskje size er innafor her.
3 participants
No description provided.