Update checkbox and radio for vanilla#34
Conversation
mshafir
left a comment
mshafir
left a comment
There was a problem hiding this comment.
Looks good, just a few minor comments, the only one you need to address somehow I think is the className
| ) | ||
| ); | ||
| display(<div>Colors: {JSON.stringify(pickedColors)}!</div>); | ||
| const choosedNumber = view( |
There was a problem hiding this comment.
chosenNumber if you want to be grammatically correct.
| return ( | ||
| <div className="flex gap-2 items-center"> | ||
| {label && <label htmlFor={stateKey}>{label}</label>} | ||
| <div className={containerClassName}> |
There was a problem hiding this comment.
What was the thinking on having the containerClassName here instead of or also on the top-level div? Folks sometimes could want to customize the placement of the label too relative to the data.
| ...props | ||
| }: RadioInputProps<T> & ViewComponentProps<string | T>) => { | ||
| return ( | ||
| <div className="flex gap-2 items-center"> |
There was a problem hiding this comment.
We shouldn't use tailwind classes in the input libs, the places they are used won't necessarily have tailwind set up and we don't want to require that.
I would like to more generally come up with a succinct solution for handling labels and other wrappers ad-hoc without needing to build them in to the inputs, but it is a bit tricky to figure out a good solution for it. For now it might be simplest to expose containerClassName and separately checkListClassName though I don't love having all the different className props
There was a problem hiding this comment.
className?: {
container?: string;
wrapper?: string;
label?: string;
item?: {
wrapper?: string;
input?: string;
label?: string;
};
};
I defined className like above
2 participants
Description
Update checkbox and radio for vanilla
What is the purpose of this pull request?