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

Alert #237

Closed
12 of 13 tasks
MrSkippy opened this issue Sep 23, 2024 · 12 comments · Fixed by #326
Closed
12 of 13 tasks

Alert #237

MrSkippy opened this issue Sep 23, 2024 · 12 comments · Fixed by #326
Assignees
Labels
new component Adding a new component to the library react Issue involving a ReactJS component

Comments

@MrSkippy
Copy link
Contributor

MrSkippy commented Sep 23, 2024

Omschrijving

Belangrijk bericht dat informeert over de huidige activiteit van de gebruiker.

Zie comment over Heading styling in Alerts.

  • Alerts hebben altijd iconen per type, zie design
  • Een type moet gekozen worden, mandatory

Definition of ready

  • Code check op community component gedaan door lux devs
  • Styling check op community component gedaan door Aline
  • Plan voor missende features of bugs van het community component beschreven in het ticket
  • Component tokens zijn beschikbaar
  • Ticket doorgenomen met Vlad & Mark

Definition of done

Component

  • Component is beschikbaar in de index
  • Design tokens worden gebruikt

Storybook

  • Playground story
  • States en properties stories
  • Korte beschrijving van het component
    • Wat doet het
    • Basis van het component (bijv Utrecht)
    • Aanpassingen en/of uitbreidingen
    • Relatie met andere componenten (bijv. Form field label en Form field)

Tests

  • Het component rendert
  • Aanpassingen en/of uitbreidingen werken (zie ook Testing components)
  • Visual regression test (door LUX)

Links

@MrSkippy MrSkippy converted this from a draft issue Sep 23, 2024
@MrSkippy MrSkippy added new component Adding a new component to the library react Issue involving a ReactJS component labels Oct 1, 2024
@AlineNap
Copy link
Contributor

AlineNap commented Oct 7, 2024

Hoe wordt de heading van de utrecht alert nu gestijlt? Welke tokens hebben jullie devs hiervoor nodig?
Wat als we proporty toe willen voegen waarbij je kunt kiezen om wel of geen heading toe te voegen? Is dat een feature request? --> Antwoord is nee.

@remypar5
Copy link
Contributor

remypar5 commented Oct 8, 2024

Hoe wordt de heading van de utrecht alert nu gestijld? Welke tokens hebben jullie devs hiervoor nodig?

Besproken is het volgende:
We gaan dit in eerste instantie oplossen in de LuxAlert. Dit kan vrij simpel door het volgende toe te passen in de CSS van het component:

.alert {
  --utrecht-heading-1-color: var(--utrecht-alert-color);
  --utrecht-heading-1-font-size: var(--lux-alert-heading-font-size);
  --utrecht-heading-1-line-height: var(--lux-alert-heading-font-size);
}

Dit passen we toe op elk kopniveau.

@MrSkippy
Copy link
Contributor Author

@AlineNap

  • Hoe werken iconen?
  1. Zoals bij Utrecht: Je moet zelf als developer altijd een icoon toevoegen?
  2. Standaard per type: De Alert van type warning heeft het icoon met het driehoekje, etc?
  3. Type 2 met mogelijkheid van eigen icoon: als je in de property icon een icoon meegeeft dan wordt die getoon ipv de standaard.
  • Is het mogelijk om geen type mee te geven?

@AlineNap
Copy link
Contributor

AlineNap commented Oct 18, 2024

@MrSkippy type 2. Zoals in de LUX Figma en in ons Webcomponent.

Dat geen type hebben we vaker besproken. Qua design zou het niet moeten kunnen. Hoe het bij het webcomponent is zou top zijn.

Qua naamgeving van de varianten vind ik lastig. Vind ok van utrecht niet heel duidelijk. In LUX Figma zitten andere nnaamgevingen en NLDS biblotheek heeft ook weer andere. Hoeveel impact heeft het als we deze naamgeving aanpassen in de toekomst?

@AlineNap
Copy link
Contributor

@remypar5 @MrSkippy nog terugkomend op de heading. Is dat niet gewoon heading component erin zetten en dan appearance H-X? Of kun je dat niet als standaard zetten op de alert? In dat geval zijn de alert component tokens voor de heading wel nodig.

@MrSkippy
Copy link
Contributor Author

MrSkippy commented Oct 22, 2024

@remypar5 @MrSkippy nog terugkomend op de heading. Is dat niet gewoon heading component erin zetten en dan appearance H-X? Of kun je dat niet als standaard zetten op de alert? In dat geval zijn de alert component tokens voor de heading wel nodig.

De developer zet zelf de <Heading> erin, alleen een Heading in een Alert wordt altijd hetzelfde vormgegeven, dus niet de Heading-styling.

@AlineNap
Copy link
Contributor

AlineNap commented Oct 22, 2024

Oke jullie doen iets in de code waardoor elke heading die je in de alert zet de alert.heading.* tokens krijgt toegepast?
Heb een PR ingeschoten om deze component tokens toe te voegen, met prefix lux.

@AlineNap
Copy link
Contributor

AlineNap commented Oct 28, 2024

2 vragen voor de devs:

  1. In code van utrecht voor de alert heten de verschillende varianten type. Is het vanuit css/ naming-convention logisch dat geen appearance is zoals bij de button? Is type iets anders?
  2. Is het een breaking change/ grote aanpassing als wij de waarde van type qua naamgeving veranderen in loop van de tijd? *Ik heb nu keuze uit 3 verschillende values en weet niet welke beste is.
    a. utrecht waard: ok, info, error, warning
    b. lux figma waarde: success, info, error, warning
    c. NLDS figma waarde: positive, informative, negative, warning

@MrSkippy
Copy link
Contributor Author

MrSkippy commented Oct 28, 2024

  1. Er zijn twee gedachtes die je daarop los kan laten:
    1. De waarden success, info, error, warning zijn het type alert, zoals text, number het type van een input aangeven, ook de manier waarop het aan een screenreader wordt doorgegeven kan daar aan gekoppeld zijn, bijv. aria-live="polite" bij info...
      Een appearance-property zal dan meer een styling-optie zijn, bijv. een "outline"-variant van de alert.
    1. De waarden success, info, error, warning zijn alleen een styling-optie, dan is appearance imho de beste optie.
  1. b. is het meest gebruikt.

@AlineNap
Copy link
Contributor

AlineNap commented Oct 28, 2024

@MrSkippy kun je je gedachten checken in de code van Utrecht? Is type nu gekoppeld aan die aria-live=“polite” etc? Want dan is daar het argument ;)

@AlineNap
Copy link
Contributor

Er loopt nog een discussie over de waarde van de types, welke gaan we toepassen?
Advies uitgevraagd in de NLDS community

Argument per keuze in volgorde:
2.a. Documentatie overnemen van utrecht, meest clean code (import - export zonder wijzigingen)
2.b. In de toekomst overgaan naar nl-alert met zo min mogelijk wijzigingen. Het is alleen niet definitief dat dit de nl waardes zijn.
2.c. Wat onze gebruikers nu gewend zijn, (mogelijk groeit nl variant hierheen, is dat dan onze input).

@MMeijerink MMeijerink self-assigned this Oct 29, 2024
@MMeijerink MMeijerink linked a pull request Nov 1, 2024 that will close this issue
3 tasks
@AlineNap
Copy link
Contributor

AlineNap commented Nov 6, 2024

Design review 6 November

Component

  1. De heading lijkt geen line-height te hebben of die steekt boven zijn eigen div uit. Zie screenshot 1.
  2. In dark mode mag de paragraph kleur zwart blijven. Pas hiervoor de token utrecht.alert.color voor toe. Zie screenshot 2.

Documentatie

  1. Graag type als titel ipv variant boven de voorbeelden zodat het overeenkomt met de playground en nog de naam erboven dus success, error, warning etc.

Vragen voor LUX devs

  1. In ons web component gebruiken we type ok en in react success. Hoe kijken jullie daar tegenaan? Gaan we nog iets met de webcomponenten doen?

Image

Image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new component Adding a new component to the library react Issue involving a ReactJS component
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants