generated from nl-design-system/example
-
Notifications
You must be signed in to change notification settings - Fork 1
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(components-react): add visual baseline to all formfield components #347
Merged
MrSkippy
merged 13 commits into
main
from
344-chore-add-visual-regression-tests-to-all-formfield-components
Jan 3, 2025
Merged
test(components-react): add visual baseline to all formfield components #347
MrSkippy
merged 13 commits into
main
from
344-chore-add-visual-regression-tests-to-all-formfield-components
Jan 3, 2025
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
9 tasks
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
remypar5
requested changes
Dec 19, 2024
packages/components-react/src/form-field-radio-group/FormFieldRadioGroup.tsx
Outdated
Show resolved
Hide resolved
packages/storybook/src/react-components/form-field-checkbox/form-field-checkbox.stories.tsx
Show resolved
Hide resolved
...ges/storybook/src/react-components/form-field-description/form-field-description.stories.tsx
Show resolved
Hide resolved
packages/storybook/src/react-components/form-field-label/form-field-label.stories.tsx
Show resolved
Hide resolved
...ges/storybook/src/react-components/form-field-radio-group/form-field-radio-group.stories.tsx
Show resolved
Hide resolved
...s/storybook/src/react-components/form-field-radio-option/form-field-radio-option.stories.tsx
Show resolved
Hide resolved
packages/storybook/src/react-components/form-field-select/form-field-select.stories.tsx
Show resolved
Hide resolved
packages/storybook/src/react-components/form-field-textbox/form-field-textbox.stories.tsx
Show resolved
Hide resolved
packages/storybook/src/react-components/form-field/form-field.stories.tsx
Show resolved
Hide resolved
AlineNap
reviewed
Dec 19, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Form field label
Top, geen opmerkingen
Form field description
- De component tokens voor de kleur van invalid en valid staan op grijs. Hoe kan het dat deze dan toch rood en groen zijn bij de voorbeelden, onderaan in Storybook. In de playground werkt het goed.
- Zouden wij invalid en valid eruit kunnen halen?
Form field error message
- Wat is de variant distanced? Hebben wij die nodig?
Textbox
- Bij invalid zou de border rood moeten
- Huisnummer veld zou korter mogen, zoals Utrecht dat ook heeft. Er is vanuit Logius nog niet nagedacht hoe al die voorbeelden eruit zouden moeten zien die zijn toegevoegd. Wat is voordeel van deze voorbeelden? Misschien beter om ze eruit te halen?
- De focus visible voorbeeld heeft geen focus outline.
- Ik mis voorbeelden van de verschillende states in Storybook
Select
- Er mist een icon aan de linkerkant
Select (documentatie)
- In de voorbeelden van de states is de value van de select gebruikt om de state aan te duiden, als je een andere state selecteert luister het daar niet naar. Fijner is om met een titel boven de select de state aan te duiden net als utrecht doet: https://nl-design-system.github.io/utrecht/storybook/?path=/docs/css_css-select--docs.
- In het voorbeeld heeft de hover een dikke border maar als ik over de select heen hover wordt de border niet dik. Heeft dat te maken met ticket utrecht?
- Wordt select an option ook doorgegeven in het component? Dan beter om die tekst in het Nederlands te doen.
Form field
- Met de utrecht.form-field.row-gap zou ik spacing verwachten tussen label, description en de error-message.
Form field radio option
- Radiobutton is 24px, zou moeten luisteren naar utrecht.radio-button.size (18px). Wat is jullie voorstel?
- Zelfde voor de dot in het midden die zou moeten luisteren naar utrecht.radio-button.icon.size (10px).
- De radio lijkt niet gecentreerd met de eerste regel van het label. Zie screenshot
Form field radio group
- Invalid heeft hier de error message bovenin. Graag 1 lijn trekken bij alle formfields, of boven de input of na de input.
- Tussen de radio options hoort lux.radio-group.row-gap(paars) toegepast te zijn dat gaat goed maar deze staat los van de spacing tussen het label, de description en de lux.radio-group zelf. Zie screenshot.
Losse vragen
- Wat is het verschil tussen form field en from-field textbox?
Form field checkbox
- Graag spacing tussen de error message en checkbox toevoegen die gelijk staat aan de utrecht.form-field.row-gap
- Waar komt de with target variant vandaan? Zie dezelfde vraag bij checkbox los.
…e-add-visual-regression-tests-to-all-formfield-components
remypar5
reviewed
Jan 2, 2025
remypar5
previously approved these changes
Jan 2, 2025
remypar5
approved these changes
Jan 3, 2025
MrSkippy
deleted the
344-chore-add-visual-regression-tests-to-all-formfield-components
branch
January 3, 2025 08:57
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contents
Add visual tests baseline to all formfield components
Checklist
Changesets are createdDefinition of Done is checked