Merge pull request #139 from mondaycom/moshe/combobox_component_fixes

Moshe/combobox component fixes

package.json

@@ -1,6 +1,6 @@
 {
   "name": "monday-ui-react-core",
-  "version": "0.3.37",
+  "version": "0.3.38",
   "description": "Official monday.com UI resources for application development in React.js",
   "main": "dist/main.js",
   "scripts": {

src/components/Combobox/Combobox.jsx

@@ -11,7 +11,7 @@ import Button from "../Button/Button";
 import useListKeyboardNavigation from "../../hooks/useListKeyboardNavigation";
 import "./Combobox.scss";
 
-const renderOption = (index, option, isActive, onOptionClick, onOptionHover) => {
+const renderOption = (index, option, isActive, isActiveByKeyboard, onOptionClick, onOptionHover) => {
   const { id, leftIcon, rightIcon, label, iconSize = 16, disabled, selected, ariaLabel } = option;
   return (
     // eslint-disable-next-line jsx-a11y/no-noninteractive-element-interactions
@@ -21,8 +21,13 @@ const renderOption = (index, option, isActive, onOptionClick, onOptionHover) =>
       ariaLabel={ariaLabel}
       id={`combox-item-${id}`}
       onMouseEnter={onOptionHover}
-      onClick={event => onOptionClick(event, index)}
-      className={cx("combobox-option", { disabled, selected, active: isActive })}
+      onClick={event => onOptionClick(event, index, option, true)}
+      className={cx("combobox-option", {
+        disabled,
+        selected,
+        active: isActive,
+        "active-outline": isActiveByKeyboard && isActive
+      })}
     >
       {leftIcon && (
         <Icon
@@ -54,14 +59,29 @@ const Combobox = forwardRef(
     const componentRef = useRef(null);
     const inputRef = useRef(null);
     const [activeItemIndex, setActiveItemIndex] = useState(-1);
+    const [isActiveByKeyboard, setIsActiveByKeyboard] = useState(false);
     const [filterValue, setFilterValue] = useState("");
     const mergedRef = useMergeRefs({ refs: [ref, componentRef] });
 
+    const setActiveItemIndexKeyboardNav = useCallback(
+      index => {
+        setActiveItemIndex(index);
+        setIsActiveByKeyboard(true);
+      },
+      [setActiveItemIndex]
+    );
+
     const onOptionClick = useCallback(
-      (_event, optionIndex) => {
-        onClick && onClick(options[optionIndex]);
+      (_event, index, option, mouseClick) => {
+        onClick && onClick(option);
+        setActiveItemIndex(index);
+        if (mouseClick) {
+          // set focus on input again
+          inputRef.current.focus();
+        }
+        setIsActiveByKeyboard(!mouseClick);
       },
-      [onClick, options]
+      [onClick]
     );
 
     const onOptionHover = useCallback(
@@ -71,13 +91,15 @@ const Combobox = forwardRef(
       [setActiveItemIndex]
     );
 
+    const filterdOptions = useMemo(() => {
+      return options.filter(({ label }) => !filterValue || label.includes(filterValue));
+    }, [options, filterValue]);
+
     const renderedItems = useMemo(() => {
-      return options
-        .filter(({ label }) => !filterValue || label.includes(filterValue))
-        .map((option, index) => {
-          return renderOption(index, option, activeItemIndex === index, onOptionClick, onOptionHover);
-        });
-    }, [options, filterValue, onOptionClick, activeItemIndex, onOptionHover]);
+      return filterdOptions.map((option, index) => {
+        return renderOption(index, option, activeItemIndex === index, isActiveByKeyboard, onOptionClick, onOptionHover);
+      });
+    }, [options, filterValue, activeItemIndex, isActiveByKeyboard, onOptionClick, onOptionHover]);
 
     const onChangeCallback = useCallback(
       value => {
@@ -86,15 +108,22 @@ const Combobox = forwardRef(
       [setFilterValue]
     );
 
-    const isChildSelectable = useCallback((index, currentOptions) => {
-      return currentOptions[index] && !currentOptions[index].disabled;
+    const isChildSelectable = useCallback(option => {
+      return option && !option.disabled;
     }, []);
 
     const onAddNewCallback = useCallback(() => {
       onAddNew && onAddNew(filterValue);
     }, [onAddNew, filterValue]);
 
-    useListKeyboardNavigation(inputRef, options, activeItemIndex, setActiveItemIndex, isChildSelectable, onOptionClick);
+    useListKeyboardNavigation(
+      inputRef,
+      filterdOptions,
+      activeItemIndex,
+      setActiveItemIndexKeyboardNav,
+      isChildSelectable,
+      onOptionClick
+    );
 
     const hasResults = renderedItems.length > 0;
     const hasFilter = filterValue.length > 0;

src/components/Combobox/Combobox.scss

@@ -76,7 +76,9 @@ $icon-margin: 4px;
 
       &.active {
         position: relative;
-        @include focus-style-css();
+        &.active-outline {
+          @include focus-style-css();
+        }
       }
 
       .option-label {

src/components/Combobox/__stories__/combobox.stories.js

@@ -1,4 +1,4 @@
-import React from "react";
+import React, { useState } from "react";
 import { action } from "@storybook/addon-actions";
 import { text, boolean, number, select } from "@storybook/addon-knobs";
 import { withPerformance } from "storybook-addon-performance";
@@ -15,22 +15,39 @@ import StoryWrapper from "../../../StoryBookComponents/StoryWrapper/StoryWrapper
 import "./combobox.stories.scss";
 
 export const Sandbox = () => (
-  <div className="combobox-wrapper">
-    <Combobox
-      placeholder="Search for content"
-      onAddNew={value => console.log("Add new ", value)}
-      addNewLabel={value => `+ Add new ${value}`}
-      onClick={option => console.log("Clicked on ", option.label)}
-      options={[
-        { id: "1", label: "first", leftIcon: "fa fa-star-o" },
-        { id: "2", label: "second", rightIcon: "fa fa-star-o", selected: true },
-        { id: "3", label: "disabled", rightIcon: "fa fa-star-o" },
-        { id: "4", label: "fourth", disabled: true }
-      ]}
-    />
+  <div className="container">
+    <ComboboxWrapper />
   </div>
 );
 
+const getOptions = selectedId => {
+  return [
+    { id: "1", label: "first", leftIcon: "fa fa-star-o", selected: selectedId === "1" },
+    { id: "2", label: "second", rightIcon: "fa fa-star-o", selected: selectedId === "2" },
+    { id: "3", label: "disabled", disabled: true, rightIcon: "fa fa-star-o", selected: selectedId === "3" },
+    { id: "4", label: "fourth", selected: selectedId === "4" }
+  ];
+};
+
+const ComboboxWrapper = () => {
+  const [selectedId, setSelectedId] = useState("2");
+
+  return (
+    <div className="combobox-wrapper">
+      <Combobox
+        placeholder="Search for content"
+        onAddNew={value => console.log("Add new ", value)}
+        addNewLabel={value => `+ Add new ${value}`}
+        onClick={option => {
+          console.log("Clicked on ", option.label);
+          setSelectedId(option.id);
+        }}
+        options={getOptions(selectedId)}
+      />
+    </div>
+  );
+};
+
 export default {
   title: "Components/Combobox",
   component: Combobox,

src/hooks/useListKeyboardNavigation.js

@@ -25,9 +25,9 @@ export default function useListKeyboardNavigation(
 
       if (direction === ARROW_DIRECTIONS.DOWN) {
         if (activeItemIndex >= options.length - 1) return;
-        for (let offset = 1; offset < options.length; offset++) {
+        for (let offset = 1; offset <= options.length; offset++) {
           const nextIndex = activeItemIndex + offset;
-          if (isChildSelectable(nextIndex, options)) {
+          if (isChildSelectable(options[nextIndex])) {
             newIndex = nextIndex;
             break;
           }
@@ -38,7 +38,7 @@ export default function useListKeyboardNavigation(
         } else {
           for (let offset = 1; offset <= activeItemIndex; offset++) {
             const prevIndex = activeItemIndex - offset;
-            if (isChildSelectable(prevIndex, options)) {
+            if (isChildSelectable(options[prevIndex])) {
               newIndex = prevIndex;
               break;
             }
@@ -61,8 +61,8 @@ export default function useListKeyboardNavigation(
   const onEnterClickCallback = useCallback(
     event => {
       const hasValidIndex = activeItemIndex || activeItemIndex === 0;
-      if (!onClick || !hasValidIndex || !isChildSelectable(activeItemIndex, options)) return;
-      onClick(event, activeItemIndex);
+      if (!onClick || !hasValidIndex || !isChildSelectable(options[activeItemIndex])) return;
+      onClick(event, activeItemIndex, options[activeItemIndex]);
     },
     [activeItemIndex, onClick, isChildSelectable, options]
   );