Passing a `children` function to MultiSelect behaves oddly about clay HOT 9 CLOSED

Thanks for the great feedback! I'll keep an eye out for that PR for the onClick and also the bug with the clicking on the menu.

I think for my specific case, I can get by for now and I will write it in a specific way so that when the changes are added to clay, they should be picked up by my component.

Thanks again @matuzalemsteles! You've been a great help

from clay.

One additional issue I noticed, when using the keyboard to navigate multi-select items (even with the OOTB multi-select), when you hit enter, the event propagates up to the form and submits the form.

from clay.

@bryceosterhaus Thanks for reporting these issues I'll look into this, most likely this came after we added the implementation of the new Autocomplete accessibility behavior to MultiSelect.

from clay.

@bryceosterhaus just a few details about the expected results, some of these behaviors would not be ideal to pass via render props because it would be a prop drilling issue as well because in that case you have control of the state since this is not a default behavior of MultiSelect with Autocomplete , so the default behaviors are close on click because you are selecting an option according to the accessibility rule and the focus should go back to the input with the selected value so we implement onClick OOTB because we also control other rules, but you can add your own rule when clicking on the item and ignoring the default behavior, you can add the onClick on the item and call the event.preventDefault() method that ignores the default behavior of the item.

As your case involves an Autocomplete with a checkbox, you who could follow the behavior that maintains the essence and accessibility rules of the list, clicking on the option must close the menu and return the focus to the input and clicking on the checkbox will probably always keep the open list and you can ignore the checkbox click event stopping the propagation of the event or keep the same behavior of the item, we don't need to pass the state via render props of the active state of the menu because you can control the active state of the menu, then you can call it directly like the other APIs to avoid prop drilling.

In fact in this example it could be even simpler if you want to keep the behavior of clicking on the checkbox it should be the same as clicking on the option, you can remove the methods of onChange and onClick I had added because I thought the behavior was to keep the open list.

from clay.

you can add the onClick on the item and call the event.preventDefault()

I don't think this is possible because when you clone the element internally, it will overwrite that onClick prop, so I don't think there is any way to ignore the default behavior of the item. Unless I am missing something here. This is for the MultiSelect.Item component.

from clay.

Also, how would I close and focus the multi-select from the child components? I don't see any APIs to do so, I could be missing something though.

I feel like I am possibly misunderstanding your comment, could you provide an example with the codesandbox I linked? That might give me a bit of clarity.

from clay.

So is this not a bug? Even with the OOTB MultiSelect, when you click on an item in the autocomplete drop-down it requires two clicks to actually select an item. The first click loses focus of the input, the second click selects the actual item. This seems inaccurate to me, because if you use the keyboard it is a single "enter" to select an item. Storybook shows the same behavior

from clay.

I don't think this is possible because when you clone the element internally, it will overwrite that onClick prop, so I don't think there is any way to ignore the default behavior of the item. Unless I am missing something here. This is for the MultiSelect.Item component.

Oh yes, this was supposed to be possible but I forgot we are already doing this to bypass the Autocomplete behavior which has different behavior than MultiSelect but I will add this because it is a pattern we are adding to all components now to allow for these use cases for example in the Autocomplete Item https://github.com/liferay/clay/blob/master/packages/clay-autocomplete/src/Autocomplete.tsx#L277-L285. (#5570)

So we can have something like:

<MultiSelect
  items={items}
>
  {(item) => (
    <MultiSelect.Item
      onClick={(event) => {
        event.preventDefault();

        // whatever you want here
      }}
    >
      {item.label}
    </MultiSelect.Item>
  )}
</MultiSelect>

Also, how would I close and focus the multi-select from the child components? I don't see any APIs to do so, I could be missing something though.

Oh sorry, apparently I forgot to explicitly add the types for active, defaultActive and onActiveChange in the component but is available in the component, I'll send a PR (#5570) to add the types so that it appears in the documentation too and doesn't cause a TypeScript error but that should be possible, for example.

<MultiSelect
  active={active}
  onActiveChange={setActive}
  items={items}
>
  {(item) => (
    <MultiSelect.Item
      onClick={(event) => {
        event.preventDefault();

        setActive(false);
        // whatever you want here
      }}
    >
      {item.label}
    </MultiSelect.Item>
  )}
</MultiSelect>

[...] focus the multi-select from the child components [...]

When you say that, do you mean to focus on the input of the MultiSelect when clicking, for example, on the checkbox? To do this you can add a ref to the component, for example:

<MultiSelect
  ref={inputRef}
  items={items}
>
  {(item) => (
    <MultiSelect.Item
      onClick={(event) => {
        event.preventDefault();

        inputRef.current.focus();
        // whatever you want here
      }}
    >
      {item.label}
    </MultiSelect.Item>
  )}
</MultiSelect>

So is this not a bug? Even with the OOTB MultiSelect, when you click on an item in the autocomplete drop-down it requires two clicks to actually select an item. The first click loses focus of the input, the second click selects the actual item. This seems inaccurate to me, because if you use the keyboard it is a single "enter" to select an item. Storybook shows the same behavior

Yes this is a bug, I was investigating this yesterday but without success I will continue investigating this this week, it is strange that this happens because we use the same Autocomplete component in MultiSelect, maybe it is some event or rendering, I don't know yet but I need to spend more time investigating this further.

For the use case of wanting that clicking on the checkbox should also close the menu to have the same behavior as clicking on the item, basically we would no longer need the onClick or onChange methods in the checkbox component, for example:

<MultiSelect
  sourceItems={filtered}
  items={items}
  onItemsChange={setItems}
>
  {(item) => (
    <MultiSelect.Item key={item.value} textValue={item.label}>
      <div className="autofit-row autofit-row-center">
        <div className="autofit-col mr-3">
          <ClayCheckbox
            checked={isSelected(items, item.value)}
          />
        </div>
        <div className="autofit-col">
          <span>{item.label}</span>
        </div>
      </div>
    </MultiSelect.Item>
  )}
</MultiSelect>

I created the example in codesandbox reproducing this behavior https://codesandbox.io/s/quick-start-clay-finale-forked-kfs25q. Basically the event propagation does the rest so we wouldn't need to control the states or have to implement everything.

In case you want to keep the menu open even if you click on the item or checkbox, we would need the other API I described above, basically you would have to control most of the states and implement the functionality to add the item to the items state. It would be something like this:

<MultiSelect
  sourceItems={filtered}
  items={items}
  onItemsChange={setItems}
>
  {(item) => (
    <MultiSelect.Item
      key={item.value}
      textValue={item.label}
      onClick={(event) => {
        event.preventDefault();

        setItems((oldItems) => {
          if (isSelected(items, item.value)) {
            return oldItems.filter(
              (oldItem) => oldItem.value !== item.value
            )
          }

          return [...oldItems, item];
        });
      }}
    >
      <div className="autofit-row autofit-row-center">
        <div className="autofit-col mr-3">
          <ClayCheckbox
            checked={isSelected(items, item.value)}
            onClick={(event) => {
              event.stopPropagation();

              setItems((oldItems) => {
                if (event.target.checked) {
                  return [...oldItems, item]
                }
      
                return oldItems.filter(
                  (oldItem) => oldItem.value !== item.value
                );
              });
            }}
          />
        </div>
        <div className="autofit-col">
          <span>{item.label}</span>
        </div>
      </div>
    </MultiSelect.Item>
  )}
</MultiSelect>

🚨 You would still need to wait for the PR I'm going to send to ignore the onClick behavior of the MultiSelect Item, I'm going to include that in today's release (#5570).

Hope this can help, let me know if anything is still confusing or something still missing to help your use case.

from clay.

This issue has been merged and will be released in DXP at https://issues.liferay.com/browse/LPS-188119

from clay.