Skip to content

feat(nve-modal): opprette modal komponent, fjerne dialog komponent#935

Open
amish1188 wants to merge 2 commits into
release-v5.0.0from
nve-modal
Open

feat(nve-modal): opprette modal komponent, fjerne dialog komponent#935
amish1188 wants to merge 2 commits into
release-v5.0.0from
nve-modal

Conversation

@amish1188

Copy link
Copy Markdown
Contributor

Etter diskusjonen med DS-styret ble vi enige om at komponenten skal hete nve-modal, ikke nve-dialog.

Bakgrunnen er at dialog-komponenten i Figma representerer modal-funksjonalitet, ikke en generell dialog. En modal er en type dialog, men med mer spesifikk oppførsel og bruk i bestemte scenarioer. Se mer informasjon her.

Denne PR-en oppdaterer dialog-/modal-komponenten med følgende endringer:

  • nve-dialog erstattes av nve-modal og implementerer kun typisk modal-oppførsel.
  • Ledetekst (label) er nå påkrevd, i tråd med WAI-ARIA-retningslinjene. Det finnes heller ingen scenarioer i Figma der modalen mangler ledetekst. Derfor er label nå en property i stedet for en slot, og det logges en advarsel i konsollen dersom den mangler.
  • noHeader fjernes (den skal alltid bli der).
  • icon endres fra property til slot.
  • headerActions fjernes. Det finnes ingen scenarioer i Figma der ekstra handlinger skal vises på samme rad som lukkeknappen.
  • open fjernes som property. Åpning av modalen styres utelukkende gjennom show()-metoden, i tråd med anbefalingene for det native <dialog>-elementet.
  • CSS-properties fjernes. ::part gir tilstrekkelig og mer målrettet styling, og bør dekke behovene.
  • Tar i bruk flere native properties og events fra <dialog>.
  • Bytter til native backdrop (beholder animasjon) og fjerner den egendefinerte backdroppen.
  • Oppdaterer dokumentasjonen.

@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Azure Static Web Apps: Your stage site is ready! Visit it here: https://brave-meadow-0c645bd03-935.westeurope.5.azurestaticapps.net

@lisamarimyreneNVE

Copy link
Copy Markdown
Contributor
image

Kunne desse underoverskriftene vært i bold? For å gjøre det litt lettere å lese :)

Comment on lines +59 to +67
* @csspart base - Dialog-elementet
* @csspart overlay - Overlegget bak dialogen - den skal ikke styles
* @csspart panel - Dialogens panel
* @csspart header - Headeren
* @csspart title - Tittelen
* @csspart header-actions - Header handlinger
* @csspart close-button - Lukk-knappen
* @csspart body - Innhold
* @csspart footer - Footer

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kan "-" fjernes her? Det ser rart ut i tabellen:

Image

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Samme for "Spor" og "Hendelser" :)

@lisamarimyreneNVE

lisamarimyreneNVE commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Veldig bra 😄

}

/**
* En modal dialog for å vise viktig innhold eller handlinger som krever brukerens oppmerksomhet.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ser ut som innholdet blir kuttet her:

Image

Kanskje det er noe med formatteringen som gjør at ikke alt synes?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bra sett!

Comment thread doc-site/components/nve-modal.md Outdated
facilisis nisi. Phasellus eget faucibus mauris, in tincidunt mi. Curabitur urna tortor, semper vel facilisis vel,
aliquam a risus. Praesent eu sapien ornare, ultrices neque id, tincidunt metus.
<div slot="footer">
<nve-button variant="primary" onclick="this.parentElement.parentElement.hide()">Ok</nve-button>

@malingranlynve malingranlynve Jul 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jeg ser nve-modal eksponerer en funksjon som heter close(), men i eksemplene her brukes hide(). Kanskje litt forvirrende for de som skal lese eksempelet, ettersom hide() ikke finnes på komponentet. Og den blir ikke lukket i eksempelet.

Comment thread doc-site/components/nve-modal.md Outdated
facilisis nisi. Phasellus eget faucibus mauris, in tincidunt mi. Curabitur urna tortor, semper vel facilisis vel,
aliquam a risus. Praesent eu sapien ornare, ultrices neque id, tincidunt metus.
<div slot="footer">
<nve-button variant="primary" autofocus onclick="this.parentElement.parentElement.hide()">Ok</nve-button>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Samme her med hide() og close()

Comment thread doc-site/components/nve-modal.md Outdated

Hvis modalen brukes til en handling som er vanskelig å angre, som sletting av data eller bekreftelse av en viktig handling, kan det være lurt å sette fokus på det minst destruktive valget først, for eksempel en avbryt-knapp. Hvis modalen derimot bare gir ekstra informasjon eller lar brukeren gå videre i en prosess, kan det være mer hensiktsmessig å sette fokus på handlingen som oftest brukes, som en «OK»- eller «Fortsett»-knapp.

Når modalen lukkes, returneres fokus som til elementet som åpnet den.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kanskje skrive om setningen til noe som: Når modalen lukkes, returneres fokus til elementet som åpnet den.

Comment thread doc-site/components/nve-modal.md Outdated

</CodeExamplePreview>

### Autofokus et interaktiv element i modalen

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Autofokus på et interaktivt element i modalen*


Alle modaler bør ha et synlig element med knapp-oppførsel som lukker modalen, for eksempel en lukkeknapp eller en avbryt-knapp. Derfor bør lukkeknappen som standard ikke fjernes.

<span class="highlight">nve-modal</span> setter <span class="highlight">aria-labelledby</span> på det native modale <span class="highlight">&lt;dialog&gt;</span>-elementet og kobler dette til <span class="highlight">label</span>-propertyen i <span class="highlight">nve-modal</span>. Derfor er <span class="highlight">label</span> påkrevd.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ekstremt pirk: Men kunne vi omformulert denne setningen? Vi sier at label er påkrevd, men implementasjonen sier at en label som ikke finnes kun gir en warning, så teknisk sett er det ikke påkrevd.

Vi kunne kanskje formulert den siste setningen noe som: "Derfor skal label alltid settes for å sikre et tydelig tilgjengelig navn for skjermlesere."

@amish1188 amish1188 Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ja, fordi vi kan ikke 'tvinge' folk til å bruke påkrevde properties i Lit. så den er påkrevd fra et tilgjengelighetsperspektiv, men ikke programmatisk, på en måte. vi gjør det samme i mange andre komponenter.

jeg viser bare et varsel her, slik at folk legger merke til det når noe mangler. syns du fortsatt jeg ikke bør nevne det?

Comment thread doc-site/components/nve-modal.md Outdated
<CodeExamplePreview>

```html
<nve-button onclick="this.nextElementSibling.show()">Vis kompakt modalen</nve-button>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kanskje bedre formulert som: "Vis kompakt modal"


</CodeExamplePreview>

## Retningslinjer

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bare en ide: Kunne det kanskje gitt mening å sette retningslinjer under eksemplene? Det er supert med god og mye dokumentasjon, men for de som bare vil se eksemplene raskt blir det litt mye scrolling. Hva tenker dere?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jeg tenkte faktisk akkurat det samme. men da må vi kanskje også oppdatere det i alle andre komponenter.

jeg tenkte at vi kanskje kunne få det på plass i en ny implementasjon av dok-siden?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enig at eksempler kunne stått først, det er det folk vil se raskest når de går inn på et komponent

@malingranlynve

Copy link
Copy Markdown
Contributor

Bra jobbet Marcin!! 🌞

@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Azure Static Web Apps: Your stage site is ready! Visit it here: https://brave-meadow-0c645bd03-935.westeurope.5.azurestaticapps.net

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants