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

test(react-components): add visual baselines for Headings #307

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

MrSkippy
Copy link
Contributor

@MrSkippy MrSkippy commented Oct 18, 2024

Contents

Visual tests en design tokens toegevoegd voor o.a:

  • Headings
  • HeadingGroup
  • PreHeading

Code cleanup voor o.a:

  • Paragraph

Checklist

  • New features/components and bugfixes are covered by tests
  • Changesets are created storybook only
  • Definition of Done is checked

Copy link

vercel bot commented Oct 18, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lux ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 20, 2024 1:37pm

@remypar5 remypar5 marked this pull request as ready for review December 5, 2024 10:46
@remypar5 remypar5 self-assigned this Dec 5, 2024
@remypar5 remypar5 changed the title chore(misc) Visual tests en design tokens toegevoegd test(react-components): Visual tests voor Headings Dec 6, 2024
@remypar5 remypar5 changed the title test(react-components): Visual tests voor Headings test(react-components): add visual baselines for Headings Dec 6, 2024
@AlineNap AlineNap self-requested a review December 19, 2024 09:47
Copy link
Contributor

@AlineNap AlineNap left a comment

Choose a reason for hiding this comment

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

  • De line-height van de pre-heading staat op normal (18px), die mag naar utrecht.pre-heading.line-height luisteren.
  • Zelfde voor de headings, daar lijkt de line-heigth ook op normal te staan.
  • Geen must wel fijn: Gezien we op utrecht geen row-gap toevoegen zou het fijn zijn die token toe te voegen met prefix lux zodat er 4px spacing tussen gezet kan worden.

@remypar5
Copy link
Contributor

De line-height van de pre-heading staat op normal (18px), die mag naar utrecht.pre-heading.line-height luisteren.

Deze verwijzing is er ook en wordt ook netjes toegepast. Het pad is als volgt:

  1. utrecht.pre-heading.line-height
  2. lux.line-height.body.default
  3. viewport.line-height.body.default
  4. lux.line-height.large-scale.sm
  5. 1.75rem

@remypar5
Copy link
Contributor

Zelfde voor de headings, daar lijkt de line-height ook op normal te staan.

Ook hier zijn de juiste tokens toegepast. Het pad is als volgt (voor heading.level-1):

  1. utrecht.heading-1.line-height
  2. lux.line-height.heading.level-1
  3. viewport.line-height.heading.level-1
  4. lux.line-height.large-scale.xl
  5. 3.5rem

@remypar5
Copy link
Contributor

Geen must wel fijn: Gezien we op utrecht geen row-gap toevoegen zou het fijn zijn die token toe te voegen met prefix lux zodat er 4px spacing tussen gezet kan worden.

Waar wil je dit zien? Deze PR beslaat 3 componenten. 9 als je de aparte heading levels mee rekent

@remypar5 remypar5 requested a review from AlineNap December 19, 2024 14:04
@AlineNap
Copy link
Contributor

AlineNap commented Dec 19, 2024

reactie op line-hieght

Gek, je hebt gelijk nu staat wel de line-height er goed op. Ook bij de heading-group.

Ik had screenshots gemaakt ik ben niet gek, 😇. Maar er staat ook een I'tje bij de pre-heading wellicht was daar iets mee.

line-height pre-heading line-height heading

@AlineNap
Copy link
Contributor

Waar wil je dit zien? Deze PR beslaat 3 componenten. 9 als je de aparte heading levels mee rekent

Op de heading group tussen de pre-heading en de heading. Dit is de aanvraag bij utrecht, maar dan dus met lux als prefix.

AlineNap
AlineNap previously approved these changes Dec 19, 2024
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.

Chore: Visual tests en design tokens toegevoegen aan componenten
3 participants