-
Notifications
You must be signed in to change notification settings - Fork 187
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
Checkboxes should use onChange
, not onClick
(or at least have the option)
#531
Comments
What's the practical problem you are facing exactly, or is it just a preference you have? There are other considerations that went into the core Checkbox primitive that the native input doesn't capture. |
I have a few custom react hooks that lets me connect my inputs to the url with as few opinions regarding react as possible. Here's an example: /** Use this to sync an input directory to the url */
export const useAutoSyncToUrl = (targetSearch?: SearchPairSet) => {
const { search, setSearch } = useTargetSearchParams(targetSearch);
/** Downshift: Returns props for inputs in order to sync properly */
const register = (name: string, value: string) => {
return {
name,
value,
onChange: autoSetSearch({ search, setSearch }, name),
checked: search.getAll(name).includes(value),
};
};
return register;
};
/** Curried function (function that returns a function)
*
* Returns an `onChange` handler that automatically syncs inputs to URLSearchParams
*
* Will absolutely not work if not used in a form */
export const autoSetSearch =
(searchPair: SearchPairSet, key: string) =>
(event: React.ChangeEvent<HTMLInputElement>) => {
if (event.target.form) {
const form = new FormData(event.target.form);
const values = form.getAll(key).map(String); // NOTE: `values` here already has the filtered values, no need to filter ourselves like most JS libraries
const newSearch = new URLSearchParams(searchPair.search);
newSearch.delete(key);
values.forEach((val) => newSearch.append(key, val));
searchPair.setSearch(newSearch);
} else {
console.warn(
`Input with key ${key} was used outside of a form. Don't do that.`
);
}
}; then it's used like this: const register = useAutoSyncToUrl();
...
{companyNames.map((code) => (
<label htmlFor={`company-name-${code}`} key={code}>
Company: {code}
<input
id={`company-name-${code}`}
type="checkbox"
{...register("company", code)}
/>
</label>
))} it works great for vanilla inputs, both checkboxes and radios, but won't work with radix checkboxes because of the way the buttons interact (or rather, don't interact) with the underlying html form. My team/company greatly prefers this way of handling data; let the html do what it does well, and hook into the results, rather than re-invent the logic that html already does for you. See the "NOTE" comment |
In this
onChange
code:values
should be a running list of all values in the form for the given name.when you use a "button", they don't behave the same and you don't get any new "values", you just get the current ones.
a native input event will include the new value or automatically remove the selected one.
example:
onChange
event.target.form's values give you["11","12","13","14"]
["11","13","14"]
no need to reinvent the wheel in javascript to parse the values (as is the modern way for some reason), you should be able to lean into the platform for this.
The text was updated successfully, but these errors were encountered: