Skip to content
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

Consistent components V1 #2221

Open
1 task
mimarz opened this issue Aug 12, 2024 · 10 comments
Open
1 task

Consistent components V1 #2221

mimarz opened this issue Aug 12, 2024 · 10 comments
Labels
☂️ epic Issues ready 🕵️ investigate Needs investigation react @digdir/designsystemet-react

Comments

@mimarz
Copy link
Collaborator

mimarz commented Aug 12, 2024

Make sure we have a consistent naming for sub-components where a list of components is expected.

For example in Tabs we use .List for a list of children while in Pagination we use Content for a list for children.

  • Investigate what naming we have used where and if it makes sense to do so.
  • Document discrepancies.
  • Decide on which components to rename.

Tasks

  1. react
@mimarz mimarz added react @digdir/designsystemet-react 🕵️ investigate Needs investigation labels Aug 12, 2024
@Barsnes
Copy link
Member

Barsnes commented Aug 12, 2024

Tab.List corresponds to its role="tablist", which makes sense to me.
Pagination.Content should probably be .List since in renders an ul

@mrosvik mrosvik added this to the V1 milestone Aug 12, 2024
@mimarz mimarz changed the title Consistent component api for expected list of children. Consistent component api Aug 16, 2024
@mimarz mimarz added the ☂️ epic Issues ready label Aug 16, 2024
@Barsnes
Copy link
Member

Barsnes commented Aug 16, 2024

.Root

Komponenter som har .Root ved skrivande tid:

  • Accordion
  • Breadcrumbs
  • DropdownMenu
  • ErrorSummary
  • List
  • Modal
  • Pagination (med del-komponenter)
  • Popover
  • Tabs
  • ToggleGroup

Komponenter som har deler uten del-komponenter:

  • Card (wrapperen rundt alt)
    • Denne rendrer ein <div>, eller elementet du asChilder til. Denne er god kandidat for .Root
  • Table (wrapperen rundt alt)
    • Denne rendrer <table>, og kunne nok ha vært .Root, ellers hadde det blitt Table.Table

Andre ting som burde bli sett på

Generelle navn på props, navn og slikt.

Komponenter som kan ha navneendring

  • NativeSelect
    • Denne rendrer ein native <select>, foreslår Select som navn
  • Textfield
    • Denne rendrer ein native <input>, foreslår Input som navn
    • Me har ikkje endra navn på <textarea>
  • DropdownMenu
    • Dette er ein dropdown, og ikkje nødvendigvis ein meny. Burde skrive retningslinjer om at det kun skal være meny, eller omdøype til det den er
  • PaginationContent
    • Denne rendrer ut <ul>. På List har me brukt .Unordered og Ordered, men dette passa ikkje på Pagination. Eg foreslår .List, då denne harmonera fint med Tabs.List
  • DropdownMenu.Content
    • Samme som over, foreslår .List

Komponenter med feil navn på andre ting

  • Accordion.Heading
    • Denne skifta frå .Header, men props og eksempel har ikkje blitt oppdatert likt
    • Kvifor har Modal og Card .Header?
      • Desse rendrer ikkje vår Heading komponent, og Accordion har skifta bort frå Header for å bedre vise det som blir rendra ut

.Trigger

Dette er for komponenta med ein del som utføre ein trigger funksjon. Eksempler er Modal og Popover
Denne navngivingen har me brukt konsistent på tvers, og eg finner ingen feil.

@eirikbacker
Copy link
Contributor

eirikbacker commented Sep 10, 2024

From discussion 10.09.24 - suggest moving away from .Root convention and purifying to a explicit .Context and single-word-named component as base component:

We suggest using Component.Context if there is a Context not providing additional elements.
We suggest using Component.Trigger if there is a optional wire-up element to create a trigger element.
We suggest using Component as the "base" element.

<Card>
  Content
</Card>

<ToggleGroup>
  <ToggleGroup.List>
    <ToggleGroup.Item>Item 1</ToggleGroup.Item>
    <ToggleGroup.Item>Item 2</ToggleGroup.Item>
    <ToggleGroup.Item>Item 3</ToggleGroup.Item>
  </ToggleGroup.List>
</ToggleGroup>

<Pagination>
  <Pagination.List>
    <Pagination.Item>Item 1</Pagination.Item>
    <Pagination.Item>Item 2</Pagination.Item>
    <Pagination.Item>Item 3</Pagination.Item>
  </Pagination.List>
</Pagination>

<Breadcrumbs aria-label="Du er her:">
  <Breadcrumbs.List>
    <Breadcrumbs.Item>Item 1</Breadcrumbs.Item>
    <Breadcrumbs.Item>Item 2</Breadcrumbs.Item>
    <Breadcrumbs.Item>Item 3</Breadcrumbs.Item>
  </Breadcrumbs.List>
</Breadcrumbs>

<Modal.Context>
  <Modal.Trigger>Knapp</Modal.Trigger>
  <Modal>Content</Modal>
</Modal.Context>

<Tooltip.Context>
  <Tooltip.Trigger>Knapp</Tooltip.Trigger>
  <Tooltip>Content</Tooltip>
</Tooltip.Context>

<Dropdown.Context>
  <Dropdown.Trigger>Knapp</Dropdown.Trigger>
  <Dropdown>Content</Dropdown>
</Dropdown.Context>

<Popover.Context>
  <Popover.Trigger>Knapp</Popover.Trigger>
  <Popover>Content</Popover>
</Popover.Context>

<Button popovertarget="my-popover">Knapp</Button>
<Popover id="my-popover">Content</Popover>

<Table>
  <Table.Header>
    <Table.Row>
      <Table.HeaderCell>Header 1</Table.HeaderCell>
      <Table.HeaderCell>Header 2</Table.HeaderCell>
    </Table.Row>
  </Table.Header>  
  <Table.Body>
    <Table.Row>
      <Table.Cell>Header 1</Table.Cell>
      <Table.Cell>Header 2</Table.Cell>
    </Table.Row>
  </Table.Body>
</Table>

@mimarz
Copy link
Collaborator Author

mimarz commented Sep 10, 2024

Komponenter som kan ha navneendring

  • NativeSelect

    • Denne rendrer ein native <select>, foreslår Select som navn

Sånnsett enig, men dette må tas opp med resten av teamet. Sist vi snakket om dette så var det gode argumenter fra @Febakke og @mrosvik om å beholde "native" prefiksen.

  • Textfield

    • Denne rendrer ein native <input>, foreslår Input som navn

Textfield er brukt som en fellesbetegnelse for et textfelt som består av flere elementer. Vi har idag i tillegg til <input>; label, description, errormessage, character counter og prefix/suffix i denne komponenten. Ved er behov så er det heller en ny komponent som er kun en ren Input. Dette kan være mer aktuelt om vi ønsker å gå for en FormField tilnærming #2311

  • DropdownMenu

    • Dette er ein dropdown, og ikkje nødvendigvis ein meny. Burde skrive retningslinjer om at det kun skal være meny, eller omdøype til det den er

Ja vi bør se generelt nærmere på denne komponenten, om det skal være en vanlig meny eller overflow meny.

  • PaginationContent

    • Denne rendrer ut <ul>. På List har me brukt .Unordered og Ordered, men dette passa ikkje på Pagination. Eg foreslår .List, då denne harmonera fint med Tabs.List

Enig. Vi har vel bestemt oss for å gjøre denne endringen allerede i #2395

  • DropdownMenu.Content

    • Samme som over, foreslår .List

Enig

Komponenter med feil navn på andre ting

  • Accordion.Heading

    • Denne skifta frå .Header, men props og eksempel har ikkje blitt oppdatert likt

    • Kvifor har Modal og Card .Header?

      • Desse rendrer ikkje vår Heading komponent, og Accordion har skifta bort frå Header for å bedre vise det som blir rendra ut

Ja, det er noe forvirring i navngivning her, kanskje det gikk i ball når vi drev å endret på dette i etterkant. Modal.Header bruker faktisk vår Heading komponent så 🤷‍♂️. Ideelt så hadde det vært fint om vi kunne slippet disse ekstra sub komponenter som egentlig er en vanlig Heading. Må undersøkes nærmere

.Trigger

Dette er for komponenta med ein del som utføre ein trigger funksjon. Eksempler er Modal og Popover Denne navngivingen har me brukt konsistent på tvers, og eg finner ingen feil.

Jepp, her ble vi enige om å tilby begge tilnærminger?

@mimarz
Copy link
Collaborator Author

mimarz commented Sep 10, 2024

From discussion 10.09.24 - suggest moving away from .Root convention and purifying to a explicit .Context and single-word-named component as base component:

Vi må og bestemme oss for hva som skjer dersom du bruker en komponent som ha omfavnet .Context?

  • Fallbacks?
  • Silent fail?
  • Throw console error?

@Barsnes
Copy link
Member

Barsnes commented Sep 10, 2024

From discussion 10.09.24 - suggest moving away from .Root convention and purifying to a explicit .Context and single-word-named component as base component:

Vi må og bestemme oss for hva som skjer dersom du bruker en komponent som ha omfavnet .Context?

  • Fallbacks?
  • Silent fail?
  • Throw console error?

Funker det ikkje at react kaster error når den ikkje finner context den trenger? Eller vil du ha ein meir leselig error melding?
Eg tenker at om ein komponent må ha contexten sin, så burde ting kræsje, altså ikkje funke.

@Barsnes
Copy link
Member

Barsnes commented Sep 10, 2024

Ja, det er noe forvirring i navngivning her, kanskje det gikk i ball når vi drev å endret på dette i etterkant. Modal.Header bruker faktisk vår Heading komponent så 🤷‍♂️. Ideelt så hadde det vært fint om vi kunne slippet disse ekstra sub komponenter som egentlig er en vanlig Heading. Må undersøkes nærmere

Nja, Modal.Header rendrer meir enn kun ein Heading, men det er det du sender inn som barn som havner i headingen 🫠
Kanskje me kan lage ein tilnærming om at når me bruker *.Header så er det forventa at konsument ordner med å legge inn Heading element sjølv?

@mimarz
Copy link
Collaborator Author

mimarz commented Sep 10, 2024

Ja, det er noe forvirring i navngivning her, kanskje det gikk i ball når vi drev å endret på dette i etterkant. Modal.Header bruker faktisk vår Heading komponent så 🤷‍♂️. Ideelt så hadde det vært fint om vi kunne slippet disse ekstra sub komponenter som egentlig er en vanlig Heading. Må undersøkes nærmere

Nja, Modal.Header rendrer meir enn kun ein Heading, men det er det du sender inn som barn som havner i headingen 🫠 Kanskje me kan lage ein tilnærming om at når me bruker *.Header så er det forventa at konsument ordner med å legge inn Heading element sjølv?

Trodde vi ble enige om å gjøre det slikt etter #2127, men glemte kanskje Modalen da, men ja, høres sånnset fornuftig ut, men har vi ikke noen bedre måte å løse lukke knappen på i Modal? Det @eirikbacker foreslo, eller om vi har en egen sub-component for close button?

@Barsnes
Copy link
Member

Barsnes commented Sep 10, 2024

Ja, det er noe forvirring i navngivning her, kanskje det gikk i ball når vi drev å endret på dette i etterkant. Modal.Header bruker faktisk vår Heading komponent så 🤷‍♂️. Ideelt så hadde det vært fint om vi kunne slippet disse ekstra sub komponenter som egentlig er en vanlig Heading. Må undersøkes nærmere

Nja, Modal.Header rendrer meir enn kun ein Heading, men det er det du sender inn som barn som havner i headingen 🫠 Kanskje me kan lage ein tilnærming om at når me bruker *.Header så er det forventa at konsument ordner med å legge inn Heading element sjølv?

Trodde vi ble enige om å gjøre det slikt etter #2127, men glemte kanskje Modalen da, men ja, høres sånnset fornuftig ut, men har vi ikke noen bedre måte å løse lukke knappen på i Modal? Det @eirikbacker foreslo, eller om vi har en egen sub-component for close button?

Om det Eirik foreslo funka, så tenker eg at det er beste løysingen :) men modalen skal ha både tittel og undertittel.
Så undertittelen må då enten få sin eigen komponent, som heller ikkje ligger i Heading 🤔

@eirikbacker
Copy link
Contributor

Masse nytt :) Hiver meg på i litt listeform jeg og;

Ang. Select;

  • Støtter at NativeSelect bytter navn til Select - det er det vi rendrer, og vi kaller det heller ikke NativeTextarea. Har lest diskusjonen om Select og autocomplete og alt, men da er vi egentlig på vei til Combobox, eller input med tilhørende datalist, så Select er fint tenker jeg siden det er et etablert konsept 😊

Ang. Textfield;

  • Textfield rendrer en haug med saker ja, men det gjør også Textarea og Select.
    • I Gjensidige lanserte vi Input, Label, Select og Textarea, Datepicker, ValidationMessage og HelpText som individuelle komponenter, men de kunne alle legges som barn av FormField.
    • Ikke heelt 100% fornøyd med det APIet, fordi man egentlig kunne hatt FormField.Help og FormField.Error som compound components.
    • Men fordelen var at konsument slapp å sette opp Label for/Input id og aria-describedby - det gjorde FormField. Samtidig var APIet veldig 1:1 med HTML så det føltes kjent, så Input var en input.
    • Input og co kunne da brukes utenfor FormField men siden den da ikke fant noe context fra FormField, måtte man da sette opp id og slikt selv.
    • Det gav utviklere fleksibilitet, og ikke minst mulighet til å lage egne form-elementer som de bare kunne droppe inn i FormField for å få label/input wiring hjelpetekst og error-melding ut av boksen. For oss kunne FormField egentlig kanskje bare være en Field.Context, men er litt usikker på hvordan vi vil gjøre det med error-melding siden den egentlig må tegne hele tiden - også når den er skjult - for å leses i alle skjermlesere :o
    • Tenker uansett vi burde gjøre noe med APIet på dagens Select/Textfield/Textarea, da det er flere DOM-elementer under panseret som er skjult uten å brettes ut med compound component API - gjør det vanskelig å legge til egne attributter etc

Ang. Modal;

  • Tenker vi absolutt burde bare bruke Heading og vise eksempel heller enn å lage egne Modal.Heading. Også fordi subtitle= er et design som kanskje noen skal ha, men skal så mange ha en subtitle over hovedtittel? Hvordan setter jeg riktige levels? Kanskje jeg vil ha noe sånt på Card også? Bedre å la brukerne våre bruke byggeklossene til å sette sammen slike ting selv, og heller vise noen eksempler på mønster vi anbefaler i Storybook og Figma? 😊
  • Close-button kunne kanskje blitt en egen Modal.Close - litt usikker på hvor mye folk skal style lukk etc i forhold til å hvor mye vi skal eksponere den, men det kan vi jo se litt på når vi kommer til å jobbe med Modal? 😊

Ang. komponenter som krever Context;

  • Støttes med at vi kaster error - det er ganske vanlig praksis ☺️

Ang. komponenter som krever Accordion.Heading;

  • Tenker fortsatt vi skal vurdere om det heter Accordion eller Expandable eller Collapse ref #😊 ?
  • -Er det heading i det heletatt? Dersom vi skal legge oss på å benytte native details/summary (som jeg tror er lurt på sikt), så er det ikke noe heading-level, mao kanskje forvirrende å bruke ordet Heading. Skulle det vært Expandable.Summary eller Expandable.Button eller Expandable.Trigger (tenker kanskje .Summary siden vi begynner å etablere at .Trigger er noe som ligger før en komponent og .Button kan fort forvirres med at det brukes Button under panseret, mens .Summary er faktisk inni komponenten?)
<Expandable>
  <Expandable.Summary>What is your biggest wish?</Expandable.Summary>
  <Expandable.Content>Unlimited wishes</Expandable.Content>
</Expandable>

eirikbacker added a commit that referenced this issue Sep 13, 2024
- Part of #2295 
- Wraps pagination for now, but should be looked more into #2396
- Skipping #2283 for now to avoid too large PR
- Renames `Pagination.Content` to `Pagination.List` following #2221
- Fixes so `Button` with `hidden` attribute is actually hidden ☺️
eirikbacker added a commit that referenced this issue Sep 17, 2024
eirikbacker added a commit that referenced this issue Sep 18, 2024
Part of #2295 and
#2221
Moves back link into `nav` as this make sense also from a11y perspective
after discussion with NRK

---------

Co-authored-by: Tobias Barsnes <tobias.barsnes@digdir.no>
Barsnes added a commit that referenced this issue Sep 18, 2024
part of #2221
I changed to using our `Popover`, which means we can handle testing
state there.
New API is this:
```tsx
import { DropdownMenu } from '@digdir/designsystemet-react';

// med context
<DropdownMenu.Context>
  <DropdownMenu.Trigger>Trigger</DropdownMenu.Trigger>
  <DropdownMenu>
    <DropdownMenu.Heading>Heading</DropdownMenu.Heading>
    <DropdownMenu.List>
      <DropdownMenu.Item>Item</DropdownMenu.Item>
    </DropdownMenu.List>
  </DropdownMenu>
</DropdownMenu.Context>

// uten context
<Button popovertarget="my-dropdown">Trigger</Button>
<Dropdown id="my-dropdown">
  <DropdownMenu.Heading>Heading</DropdownMenu.Heading>
  <DropdownMenu.List>
    <DropdownMenu.Item>Item</DropdownMenu.Item>
  </DropdownMenu.List>
</Dropdown>
```
eirikbacker added a commit that referenced this issue Sep 18, 2024
- Part of #2221
- Renames `Tabs.Root` to `Tabs`
- Renames `Tabs.Content` to `Tabs.Panel` as this aligns with
ARIA-standard and is a more common convention
@mimarz mimarz changed the title Consistent component api Consistent components V1 Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
☂️ epic Issues ready 🕵️ investigate Needs investigation react @digdir/designsystemet-react
Projects
Status: ☂ Epics
Development

No branches or pull requests

4 participants