fix(nve-switch): fikse bryter med riktig størrelsen pa mobil + en del funksj - #962
fix(nve-switch): fikse bryter med riktig størrelsen pa mobil + en del funksj#962amish1188 wants to merge 1 commit into
Conversation
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://brave-meadow-0c645bd03-962.westeurope.5.azurestaticapps.net |
There was a problem hiding this comment.
Pull request overview
Denne PR-en oppdaterer nve-switch for å vises korrekt på smale skjermer (Issue #625) og forenkler implementasjonen ved å lene seg mer på native checkbox-atferd. Endringene omfatter både komponentkode, styling, tester og dokumentasjon.
Changes:
- Rework av switch-styling (nye CSS-variabler, justert layout/hover/fokus, ny thumb-anim) for bedre mobilvisning.
- Forenklet komponentlogikk: fjernet flere interne event handlers og synk-logikk, og håndterer nå state via native
change. - Oppdatert tester og docs i tråd med ny DOM-/klasse-struktur, samt mindre repo-opprydding i
.gitignore.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/components/nve-switch/nve-switch.component.ts |
Forenkler event-/state-håndtering og oppdaterer struktur/parts/classes. |
src/components/nve-switch/nve-switch.styles.ts |
Ny sizing-/layoutmodell med CSS-variabler og oppdatert hover/fokus/checked-stiler. |
src/components/nve-switch/nve-switch.test.ts |
Oppdaterer selektorer/forventninger til ny DOM og klassenavn. |
doc-site/components/nve-switch.md |
Utvider og oppdaterer dokumentasjon og tilgjengelighetsråd. |
.gitignore |
Rydder og ignorerer custom-elements-manifest.mjs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @state() private hasFocus = false; | ||
| @property() title = ''; // make reactive to pass through | ||
|
|
||
| @property({ type: String }) testId: string = ''; |
| this.dispatchEvent( | ||
| new CustomEvent('change', { | ||
| bubbles: true, | ||
| composed: true, | ||
| detail: { value: this.value }, //usikker om vi trenger value her | ||
| }) | ||
| ); |
| .switch__thumb { | ||
| content: ''; | ||
| position: absolute; | ||
| left: var(--left); | ||
| height: 18px; | ||
| width: 18px; | ||
| border-radius: 2rem; | ||
| translate: var(--hover-offset, 0); | ||
| z-index: 1; | ||
| background-color: var(--thumb-color); | ||
| left: var(--thumb-offset); |
| expect(label?.classList.contains('switch__label--start')).toBe(false); | ||
| }); | ||
|
|
||
| it('should apply switch--label-start class when label-position="start"', async () => { |
| ## Retningslinjer | ||
|
|
||
| - Gi alltid en tydelig <span class="highlight">label</span>. | ||
| - Ikke endre <span class="highlight">label</span> basert på bryterens tilstand. Labelen skal beskrive hva bryteren styrer, ikke hvilken handling som utføres. Bruk for eksempel «Vis info som fast label i stedet for å bytte mellom «Vis info og «Skjul info. |
| .switch__input:not(:disabled) + .switch__control:hover { | ||
| background-color: var(--control-background-hover); | ||
| } | ||
|
|
||
| .switch input[type='checkbox'] { | ||
| clip: rect(0, 0, 0, 0); | ||
| position: absolute; | ||
| .switch__input:checked:not(:disabled) + .switch__control:hover { | ||
| background-color: var(--control-background-checked-hover); |
|
Skulle du legge inn ønsket fra Åsne i denne PRen? Med ekstra label og tekst? |
Jeg skal. Kanskje klarer det i morgen |
malingranlynve
left a comment
malingranlynve
left a comment
There was a problem hiding this comment.
Switchen ser så mye bedre ut i mobilformat nå! Bra jobbet 😊
| --width: 3rem; | ||
| --thumb-size: 1.125rem; | ||
| --thumb-offset: calc((var(--height) - var(--thumb-size)) / 2); | ||
| --thumb-background: var(--color-interactive-foreground-secondary-enabled); |
There was a problem hiding this comment.
Det stemmer at vi endrer farge på switchene? Før var den --color-neutrals-foreground-subtle og nå er den --color-interactive-foreground-secondary-enabled
| ### Varianter | ||
|
|
||
| Bruk variant for å velge farge, default er standard. | ||
| Du kan bruke <span class="highlight">variant</span> for å sette farger (når bryteren er på) : |
There was a problem hiding this comment.
Jeg synes det kan være litt forvirrende med denne setningen, sånn jeg forstår den kan jeg velge en hvilken som helst farge her. Kanskje det kan formuleres mer som: Du kan bruke variant for å velge mellom to farger: default eller primary
| --hover-offset: 0px; | ||
| cursor: pointer; | ||
| font: var(--typography-label-medium-light); | ||
| color: var(--color-neutrals-foreground-primary); |
There was a problem hiding this comment.
Jeg ser også at selve labelteksten er i en annen fargetone. Tror jeg foretrekker den gamle, med mindre det er en anbefaling fra Frode eller at fargeforskjellen er for lav 🤔
Gammel
Ny
There was a problem hiding this comment.
Personlig synes jeg det er bra at den har fått større kontrast, da det er lettere å se om den er på/av 🤔
|
Litt vanskelig å forklare så jeg prøver med en video. Hvis man er i mobilformat og trykker på switchen får den ikke riktig farge, den får først riktig farge på onChange når man klikker utenfor switchen. Altså den beholder fargen den har i animasjonen switch.mp4 |
Fikser issue #625
Jeg oppdaterer bryteren slik at den vises riktig på små skjermer.
I tillegg har jeg forenklet koden litt. Har fjernet funksjonalitet som ikke trengs, som de fleste hendelseshåndtererne.
Har fjernet animasjonen på hover. Syns den ikke var så bra, men hvis folk er uenige, kan vi ta den tilbake.