Combobox: Add support for onBlur, and omit props that have no effect

[HalvorHaugan]

Also added example of usage with react-hook-form, which was the primary task originally 😅

[changeset-bot]

🦋 Changeset detected

Latest commit: 54693d7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages

Name	Type
@navikt/ds-react	Minor
@navikt/ds-css	Minor
@navikt/ds-tokens	Minor
@navikt/ds-tailwind	Minor
@navikt/aksel-icons	Minor
@navikt/aksel	Minor
@navikt/aksel-stylelint	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[HalvorHaugan]

Patch or minor? 🤔

[KenAJoh]

Leaning towards minor since lots of features are omitted from types (even if none of them worked before 😅 )

[HalvorHaugan]

I needed onBlur for the example, so I only added composeEventHandlers on that one for now. But should we add it to the other event handlers as well?

[KenAJoh]

Doesn't hurt to add it to all of them i guess, but not needed in this PR 🤷

[github-actions]

Storybook demo

89c9173b3 | 88 komponenter | 139 stories

[KenAJoh]

Adding a button with type="submit" under the combobox might make it easier to move focus and testing submit for the user 🚀

[KenAJoh]

Is combobox more aligned with the checkbox pattern of requiring "onChange" as reValidateMod? Since i'm actively making a choice to pick a new value (like "car" here), it seems like something else is wrong if the error still stands

[JulianNymark]

I'm not able to recreate this, when I've selected a value the error disappears for me (firefox).

[JulianNymark]

actually, i can get this state if i close the list options by navigating the mouse cursor up one from the first element. So nvm. I guess this would be a confusing state to end up in.

[JulianNymark]

Since combobox has a text input field, then the onChange for that should probably not cause rapid changes to the dom / form validation? (we can prevent validation on this onChange, but have it trigger on the selectedOptions onChange (to avoid the above situation).

[KenAJoh]

I get this state by only using keyboard or clicking "car" in dropdown then hitting "esc" to close menu. I guess most will click outside the menu to close it thus triggering onBlur, but would be best if it was experienced the same for both mouse and keyboard-users.

Hmm true about the onChange firing raid events, would not be optimal 🤔 Would it be possible to manually update validation-state within onToggleSelected? Something like

onToggleSelected: (value, isSelected) => if isSelected -> updateValidation()

[KenAJoh]

Seems like i didn't properly understand how the Controller function worked 🙈 Since we have full control over where field.onChange is added, we avoid rapid events like you mentioned Julian 👍

I tested with this snippet and it seems to work like expected:

• Added selectedOptions={field.value} since we have to control the combobox
• Changed to reValidateMode: "onChange",

const Example = () => {
  const {
    handleSubmit,
    control,
    formState: { errors },
  } = useForm<Inputs>({
    reValidateMode: "onChange",
    defaultValues: { transportmiddel: [] },
  });

  const onValidSubmit: SubmitHandler<Inputs> = (data) => {
    alert(data);
  };

  return (
    <form onSubmit={handleSubmit(onValidSubmit)}>
      <Controller
        control={control}
        rules={{ required: "Du må velge minst ett transportmiddel." }}
        name="transportmiddel"
        render={({ field }) => (
          <UNSAFE_Combobox
            id="transportmiddel"
            label="Hva er de kuleste transportmidlene?"
            options={options}
            isMultiSelect
            error={errors.transportmiddel?.message}
            ref={field.ref}
            name={field.name}
            selectedOptions={field.value}
            onToggleSelected={(option, isSelected) => {
              if (isSelected) {
                field.onChange([...field.value, option]);
              } else {
                field.onChange(field.value.filter((v) => v !== option));
              }
            }}
          />
        )}
      />
    </form>
  );
};

[HalvorHaugan]

I agree that we can validate on onChange here. "onChange" in this case is actually onToggleSelected. The reason for choosing onBlur at first was to avoid too early validation if you need to select multiple items. But when thinking about it, that shouldn't be a problem as long as devs don't create too clever error messages.

Let's say you have selected 1 but need to select 3. Error message should be "you need to select at least 3 items". If you remove everyting, the error should be the same. If you add one, the error should still be the same. It would only be a problem if the error said like "select 2 MORE items", but I think that's an edge case. Even then, at least the error changes only when selecting, not on every keystroke 😅

[it-vegard]

As far as I know, the teams I have had contact with about Combobox and react-hook-form use reValidateMode: "onChange".

[JulianNymark]

I like grouping all the aria props near the bottom, could be a nice convention.

[it-vegard]

Is the reason for omitting these extra attributes simply that we use our own, internal ones?
Should we Pick<InputHTMLAttributes<HTMLInputElement>, "which" | "props" | "we" | "use"> instead?
I guess there are still a lot of attributes left that we don't actually use? 🤔

[HalvorHaugan]

The reason for omitting these (the ones I added) is because we override them, so they don't work anyways. So now you will get TS error if you try to use them, instead of just silently failing.

I'm not sure how strict we should be about which props to accept. One could argue that since the input is so specialized, we should use Pick instead so that we know that we only offer props that works properly. This will avoid another "placeholder incident" where we have to say that "yes, it's supported, but not really" 😅

[HalvorHaugan]

I'll change to Pick in a new PR.