-
Notifications
You must be signed in to change notification settings - Fork 1
test: Skal ikke merges, men brukes for å lage en rask test og demonstrasjon av heading-komponent #802
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
Open
tomapedersen
wants to merge
3
commits into
main
Choose a base branch
from
test/kjapp-heading-test
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
test: Skal ikke merges, men brukes for å lage en rask test og demonstrasjon av heading-komponent #802
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| --- | ||
| layout: component | ||
| --- | ||
|
|
||
| <CodeExamplePreview> | ||
|
|
||
| ```html | ||
| <nve-heading level="1">Dette er en overskrift</nve-heading> | ||
| ``` | ||
|
|
||
| </CodeExamplePreview> | ||
|
|
||
| ## Eksempler | ||
|
|
||
| La til et par eksepler for å teste her: | ||
|
|
||
| <CodeExamplePreview> | ||
|
|
||
| ```html | ||
| <nve-heading level="5">Dette er en h5 med default styling</nve-heading> | ||
| <nve-heading level="2" typographyType="headingMedium">Dette er en h2 med headingMedium typografi</nve-heading> | ||
| ``` | ||
|
|
||
| </CodeExamplePreview> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| import { LitElement } from 'lit'; | ||
| import { unsafeStatic, html } from 'lit/static-html.js'; | ||
| import { customElement, property } from 'lit/decorators.js'; | ||
| import { INveComponent } from '@interfaces/NveComponent.interface'; | ||
| import styles from './nve-heading.styles'; | ||
|
|
||
| @customElement('nve-heading') | ||
| export default class NveHeading extends LitElement implements INveComponent { | ||
| @property({ type: String }) testId: string | undefined = undefined; | ||
| /** Heading level - Hvilket nivå overskriften skal ha */ | ||
| @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?: | ||
| | 'headingXlarge' | ||
| | 'headingLarge' | ||
| | 'headingMedium' | ||
| | 'headingSmall'; | ||
|
|
||
| static styles = [styles]; | ||
|
|
||
| constructor() { | ||
| super(); | ||
| } | ||
| protected getTagName() { | ||
| if (!this.level || this.level < 1 || this.level > 6 || isNaN(this.level)) { | ||
| return 'h1'; | ||
| } | ||
| return `h${this.level}`; | ||
| } | ||
|
|
||
| protected getDefaultTypographyType() { | ||
| switch (this.level) { | ||
| case 1: | ||
| return 'headingXlarge'; | ||
| case 2: | ||
| return 'headingLarge'; | ||
| case 3: | ||
| return 'headingMedium'; | ||
| default: | ||
| return 'headingSmall'; | ||
| } | ||
| } | ||
|
|
||
| render() { | ||
| const tagName = this.getTagName(); | ||
| return html` | ||
| <${unsafeStatic(tagName)} part="base" data-testid=${this.testId} class=${this.typographyType ? this.typographyType : this.getDefaultTypographyType()}> | ||
| <slot></slot> | ||
| </${unsafeStatic(tagName)}> | ||
| `; | ||
| } | ||
| } | ||
|
|
||
| declare global { | ||
| interface HTMLElementTagNameMap { | ||
| 'nve-heading': NveHeading; | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| import { css } from 'lit'; | ||
|
|
||
| export default css` | ||
| .headingXlarge { | ||
| font: var(--typography-heading-x-large); | ||
| } | ||
|
|
||
| .headingLarge { | ||
| font: var(--typography-heading-large); | ||
| } | ||
|
|
||
| .headingMedium { | ||
| font: var(--typography-heading-medium); | ||
| } | ||
|
|
||
| .headingSmall { | ||
| font: var(--typography-heading-small); | ||
| } | ||
| `; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| import { afterAll, describe, expect, it, } from 'vitest'; | ||
| import { fixture, fixtureCleanup } from '@open-wc/testing'; | ||
| import { html } from 'lit'; | ||
| import NveHeading from './nve-heading.component'; | ||
|
|
||
| if (!customElements.get('nve-heading')) { | ||
| customElements.define('nve-heading', NveHeading); | ||
| } | ||
|
|
||
| describe('nve-heading', () => { | ||
| afterAll(() => { | ||
| fixtureCleanup(); | ||
| }); | ||
|
|
||
| it('is attached to the DOM', async () => { | ||
| const el = await fixture<NveHeading>(html`<nve-heading></nve-heading>`); | ||
| expect(document.body.contains(el)).toBe(true); | ||
| }); | ||
| }); | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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=headingelementer som ligger i shadow-dom (det ser ut til å virke ok)Tror at
sizevil 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
sizesom navnThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.