-
Notifications
You must be signed in to change notification settings - Fork 4
Conversation
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.
LGTM, check out my comments and consider if you want to make any changes.
package.json
Outdated
@@ -1,6 +1,6 @@ | |||
{ | |||
"name": "@dotkomonline/design-system", | |||
"version": "0.14.0", | |||
"version": "0.18.0", |
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.
To be completely honest I haven't really understood the autodeploy thing. But if I'm not totally mistaken it doesn't autoincrement this file, which is why we are in this problem now. And we're currently on 0.18.0. So this commit will increment the released package to 0.19.0 and set the number here to 0.18.0
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.
boi you're probably correct
|
||
<Meta title="Forms|Radio" component={Radio} /> | ||
|
||
# Radio | ||
|
||
Our radio buttons are defined as a group. Use this over checkboxes when you only want to provide one choice to the user. For now you can only input text as labels, not stuff like icons. | ||
The radio group is created by passing an `Array` of choices as: |
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.
This is a pretty unstandard way of creating radio-groups. But I don't really mind it.
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.
Hm, I agree. I have iterated a bit through different ways of doing it and ended up with this one.
In retrospect, I think I will see if it can rather be done through React.cloneElement()
so that the structure can be
<RadioGroup onChange={onChange} groupName={groupName}>
<RadioButton value={value}>{label}</RadioButton>
<RadioButton value={value} checked>{label}</RadioButton>
</RadioGroup>
With onChange
, groupName
and checked
being optional
error?: boolean; | ||
interface RadioButtonProps extends Omit<RadioProps, 'choices' | 'onChange'> { | ||
value: string; | ||
children: React.ReactNode; |
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.
Is this the right type? I would assume it would be some sort of collection of React.ReactNode?
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.
React.ReactNode
is the default type for children
, yes.
React.ReactNode
can be almost anything, both a collection of things, but also a single element, function, string, etc.
The type for children
is usually inferred from React.FC<Props>
which adds the children
-prop.
I could perhaps remove the definition and use React.FC<Props>
, but we've had some issues
with Storybook not picking up the props with that
Props-table are no longer displayed, but should be fixed by upgrading to Storybook v6. |
🎉 This PR is included in version 0.19.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Checklist
yarn lint-check
.yarn chromatic
such that other people can review my UI changes at chromaticqa.com../src/index.ts
.About the pull request
Radio buttons' value were only retrievable through a
form
-submit.This change allows for radio buttons' value to be retrieved through an
onChange
-function.It also allows for React-elements to be set as labels.
It also adds support for a default selected value. This is useful when displaying a form that the user has already submitted, but may still update.