Conversation
da8d2a8 to
ff9cd1d
Compare
ff9cd1d to
333c4e8
Compare
♿ Accessibility audit — WCAG 2.2 AA✅ No violations across 424 stories / 11872 theme·mode combinations in Automated axe covers ~30–50% of WCAG 2.2 AA. Keyboard, screen-reader and reflow checks still need a manual pass. Full per-story JSON is in the run’s |
1641137 to
74bad31
Compare
WDeenikSURF
left a comment
There was a problem hiding this comment.
Vind het allemaal een stuk leesbaarder worden zo, zeker de lines met tientallen Tailwind classnames ❤️
Heb obviously niet alle styles vergeleken, maar heb vertrouwen in de Playwright tests die je daarvoor hebt opgezet.
Flink wat comments, maar wat kan je verwachten met een PR als deze 😬 Niet alles relevant voor deze PR, maar heb ze maar neergepend, wel goed om een keer over te hebben. Wat wel relevant is is vaak ook relevant voor meerdere files, maar heb dingen niet herhaald als ik ze vaker zag.
| @@ -0,0 +1,404 @@ | |||
| /*! Reset based on tailwindcss v4.3.1 preflight | MIT License | https://tailwindcss.com */ | |||
There was a problem hiding this comment.
Ik zie in de apps dat we daar de preflight juist skippen omdat onze design system packages reset includen via deze file.
Ik vraag me af of dat later tot problemen zou kunnen leiden, als onze resets en de preflight van een app z'n tailwind niet meer in sync zijn door een versieverschil (en de app's tailwind resets verwacht die wij niet gebruiken, of andersom). Dit kan ook met andere styling oplossingen voorkomen overigens. Allebei de resets gebruiken kan ook vervelende gevolgen hebben.
Geen idee of daar een goede oplossing voor is overigens 🤔 Dit moet bij meer design systems / component libraries spelen zou je zeggen.
Maar goed eigenlijk buiten scope van deze PR, aangezien dit ook speelde met Tailwind zelf. Zie dat op de Tailwind repo iemand precies deze vraag stelt maar zonder reacties.
| * Designer- and app-facing semantic color utilities (Tailwind-like names). | ||
| * Maps to Curve token CSS variables — not generated by Tailwind. | ||
| * Layout/spacing utilities belong in the app or Storybook Tailwind entry. |
There was a problem hiding this comment.
Kleine nitpick: ik zie nu in heel veels comments referenties naar dat we specifiek niet Tailwind gebruiken. Dat maakt sense in context van deze PR, maar voor de toekomst maakt het natuurlijk helemaal niet uit dat we eerst Tailwind gebruikten en nu niet meer.
| 'border-input dark:bg-input/30 has-[[data-slot=input-group-control]:focus-visible]:border-ring has-[[data-slot=input-group-control]:focus-visible]:ring-ring/50 has-[[data-slot][data-matches-spartan-invalid=true]]:ring-destructive/20 has-[[data-slot][data-matches-spartan-invalid=true]]:border-destructive dark:has-[[data-slot][data-matches-spartan-invalid=true]]:ring-destructive/40 h-9 rounded-md border shadow-xs transition-[color,box-shadow] in-data-[slot=combobox-content]:focus-within:border-inherit in-data-[slot=combobox-content]:focus-within:ring-0 has-[[data-slot=input-group-control]:focus-visible]:ring-3 has-[[data-slot][data-matches-spartan-invalid=true]]:ring-3 has-[>[data-align=block-end]]:h-auto has-[>[data-align=block-end]]:flex-col has-[>[data-align=block-start]]:h-auto has-[>[data-align=block-start]]:flex-col has-[>[data-align=block-end]]:[&>input]:pt-3 has-[>[data-align=block-start]]:[&>input]:pb-3 has-[>[data-align=inline-end]]:[&>input]:pe-1.5 has-[>[data-align=inline-start]]:[&>input]:ps-1.5 group/input-group relative flex w-full min-w-0 items-center outline-none has-[>textarea]:h-auto', | ||
| ); | ||
| // Styling lives in ./hlm-input-group.css; `group/input-group` stays as a hook for consumers' Tailwind. | ||
| classes(() => 'curve-input-group group/input-group'); |
There was a problem hiding this comment.
Idealiter hoeven we geen rekening te houden met of consumers Tailwind gebruiken, toch zie ik overal deze "hook for consumers' Tailwind" classnames. Kunnen we zonder? Als een hook als deze echt nodig is zodat de consumer dit component goed kan gebruiken (liever vermijden w.m.b.), is het beter om ze zelf custom classnames toe te kunnen laten voegen IMO
There was a problem hiding this comment.
Ik ben hier een beetje huiverig voor omdat dit een vraag is om aan de Angular-gebruikers te stellen. Ik stel voor om dit apart op te pakken
| * defining it here is the real source of truth and lets consumers override | ||
| * the base unit for their whole app. |
There was a problem hiding this comment.
and lets consumers override the base unit for their whole app
Willen we die flexibiliteit geven? "Dingen passen niet mooi op het scherm, maar als ik --spacing: 2px doe wel, let's go" lijkt me niet iets dat we willen?
Dat staat overigens los van of het überhaupt handig is om spacing aan rems te koppelen, maar dat is een hele andere discussie.
There was a problem hiding this comment.
In zijn algemeenheid is het aanpassen van CSS variables al mogelijk toch, dus die flexibiliteit hebben ze? Dit lijkt me meer een stuk governance, hoe strak moet het Curve team controleren of het design systeem goed wordt gebruikt
| .sbdocs-content > ul, | ||
| .sbdocs-content > ol { |
There was a problem hiding this comment.
Ik zie deze styles ook langskomen in packages/storybook-config. Waarom gebruiken we die niet in de react/angular packages?
There was a problem hiding this comment.
Dit is styling voor Storybook zelf alleen, en daarmee niet een onderdeel van de packages :)
| - React and Angular each have a `*.spec.ts` that `toHaveScreenshot`s every story (light + dark). | ||
| Baselines live in separate folders: `tests/visual/__screenshots__/react/` and | ||
| `.../angular/` (same story ids, different PNGs — never share one flat directory). | ||
| - Refresh baselines with `pnpm test:visual:update`. Optional parity: `pnpm test:visual:parity`. |
There was a problem hiding this comment.
Misschien goed om erbij te zetten dat baselines pas moeten worden bijgewerkt als zeker is dat de nieuw look intended is? Niet dat een AI agent de test als een probleem ziet en ipv de regressie te fixen de visuals update.
There was a problem hiding this comment.
Ik denk dat we die baseline sowieso zelf moeten refreshen handmatig.
Signed-off-by: Anneke Sinnema <mail@annekesinnema.nl>
Signed-off-by: Anneke Sinnema <mail@annekesinnema.nl>
Signed-off-by: Anneke Sinnema <mail@annekesinnema.nl>
Signed-off-by: Anneke Sinnema <mail@annekesinnema.nl>
Signed-off-by: Anneke Sinnema <mail@annekesinnema.nl>
Signed-off-by: Anneke Sinnema <mail@annekesinnema.nl>
Signed-off-by: Anneke Sinnema <mail@annekesinnema.nl>
Signed-off-by: Anneke Sinnema <mail@annekesinnema.nl>
Signed-off-by: Anneke Sinnema <mail@annekesinnema.nl>
Summary
Changes styling of components to using CSS Modules, instead of Tailwind. (For React components)
Type of change
Checklist
pnpm build,pnpm lint, andpnpm formatpasspnpm changeset) — required for any change to@surfnet/curve-reactor@surfnet/curve-angular; N/A for docs/CI-only changes@surfnet/curve-contracts)