radix-ui / themes

Radix Themes is an open-source component library optimized for fast development, easy maintenance, and accessibility. Maintained by @workos.
https://radix-ui.com/themes
MIT License
5.17k stars 187 forks source link

Checkboxes should use `onChange`, not `onClick` (or at least have the option) #531

Open jamesholbert-emsi opened 2 months ago

jamesholbert-emsi commented 2 months ago

In this onChange code:

  (event: React.ChangeEvent<HTMLInputElement>) => {
    if (event.target.form) {
      const form = new FormData(event.target.form);
      const values = form.getAll(key).map(String);
  }

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:

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.

vladmoroz commented 2 months ago

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.

jamesholbert-emsi commented 2 months ago

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