[Component] SelectBox #4

amnambiar · 2025-10-14T07:16:59Z

Pull request template

Description

Details of this SelectBox component and its behaviour is as per this document

Task:

Tailwind Selectbox #424

Checklist

…rch, multiselect(show all options/ x more)

…CHA-react-FE-template into nextjs-component/SelectBox

KJES4 · 2025-10-28T13:56:31Z

starter_app/src/app/components/SelectBox.tsx

+    label: string;
+    value: string;
+    type?: "standard";
+    status?: "passed" | "failed" | "pending";


Let's change status to match the status options for a property "valid" | "falsified" | "undetermined"

KJES4 · 2025-10-28T13:59:16Z

starter_app/src/app/page.tsx

            onChange={(val) => setSortValue(val as string)}
        />
+        <SelectBox
+                options={selectBoxGroupedOptions}


On my machine I am seeing a ts error on this prop. Is that showing up on your end?

SelectBox.tsx(24, 5): The expected type comes from property 'options' which is declared here on type 'IntrinsicAttributes & SelectProps'

KJES4 · 2025-10-28T14:12:16Z

starter_app/src/app/components/SelectBox.tsx

+    onChange: (values: string[]) => void;
+  }
+
+  const Select: React.FC<SelectProps> = ({


Using React.FC is a historically controversial way to define a component in React as such many project have moved away from doing this. As of TypeScript 5.1 and React 18, React.FC is now officially 'fine' but many project still do not use this. This way of defining a function also doesn't match the other ways functions were defined and it is important to keep code consistent across the codebase at times where it makes sense to do so. Is there a particular reason you chose to use React.FC here?

KJES4 · 2025-10-28T14:49:51Z

starter_app/src/app/components/SelectBox.tsx

+  }) => {
+    const [isOpen, setIsOpen] = useState<boolean>(false);
+    const [searchValue, setSearchValue] = useState<string>("");
+    const [selectedValues, setSelectedValues] = useState<string[]>([]); // values of slected options


The state for the selectedValues is state that will impact more than just this component so it should be lifted up out of this component and passed through as a prop just like what was done in the dropdown component.

KJES4 · 2025-10-28T15:02:18Z

starter_app/src/app/components/SelectBox.tsx

+    const chipsContainerRef = useRef<HTMLDivElement>(null);
+
+    const theme = {
+      textOnSurface: "text-gray-900 dark:text-gray-100",


onSurface is already a CSS variable that is defined in the global.css file. It is further defined as a css token and it already has both a light and dark option defined. This doesn't need to be redefined in this file.

KJES4 · 2025-10-28T15:02:54Z

starter_app/src/app/components/SelectBox.tsx

+
+    const theme = {
+      textOnSurface: "text-gray-900 dark:text-gray-100",
+      bgContainer: "bg-white dark:bg-gray-700",


container is already a CSS variable that is defined in the global.css file. It is further defined as a css token and it already has both a light and dark option defined. This doesn't need to be redefined in this file.

KJES4 · 2025-10-28T15:03:25Z

starter_app/src/app/components/SelectBox.tsx

+    const theme = {
+      textOnSurface: "text-gray-900 dark:text-gray-100",
+      bgContainer: "bg-white dark:bg-gray-700",
+      bgContainerHigh: "bg-gray-100 dark:bg-gray-800",


containerHigh is already a CSS variable that is defined in the global.css file. It is further defined as a css token and it already has both a light and dark option defined. This doesn't need to be redefined in this file.

KJES4 · 2025-10-28T15:03:45Z

starter_app/src/app/components/SelectBox.tsx

+      textOnSurface: "text-gray-900 dark:text-gray-100",
+      bgContainer: "bg-white dark:bg-gray-700",
+      bgContainerHigh: "bg-gray-100 dark:bg-gray-800",
+      borderOutline: "border-gray-300 dark:border-gray-600",


outline is already a CSS variable that is defined in the global.css file. It is further defined as a css token and it already has both a light and dark option defined. This doesn't need to be redefined in this file.

KJES4 · 2025-10-28T15:09:32Z

starter_app/src/app/components/SelectBox.tsx

+    const inputRef = useRef<HTMLInputElement>(null);
+    const chipsContainerRef = useRef<HTMLDivElement>(null);
+
+    const theme = {


Please remove this theme object and define all the styles inline, all but one of the style options in this theme options have already been defined in the global stylesheet or are only applied in one location.

KJES4 · 2025-10-28T16:17:50Z

starter_app/src/app/components/SelectBox.tsx

+    };
+
+    return (
+      <div ref={componentRef} className="relative w-full font-inter">


Why is the font set to inter here?

KJES4 · 2025-10-28T16:20:37Z

starter_app/src/app/components/SelectBox.tsx

+        }
+      };
+      document.addEventListener("mousedown", handleClickOutside);
+      return () => document.removeEventListener("mousedown", handleClickOutside);


If you used a 'click' event listener in dropdown why did you choose to use a 'mousedown' click event in this listener?

KJES4 · 2025-10-28T16:52:57Z

starter_app/src/app/components/SelectBox.tsx

+    showAllSelected = false,
+    onChange = () => {},
+  }) => {
+    const [isOpen, setIsOpen] = useState<boolean>(false);


Can you name these state variables to [open, setOpen] that way we are ensuring consistent naming in all the components? The dropdown component has the naming for this similar state that I am requesting.

KJES4

I left some comments but this is still in review.

KJES4 · 2025-10-29T13:02:48Z

starter_app/src/app/components/SelectBox.tsx

+              e.stopPropagation();
+              if (!isOpen) setIsOpen(true);
+            }}
+            placeholder={placeholderVisible ? placeholder : ""}


Can you explain to me what this conditional statement is for?

KJES4 · 2025-10-29T13:11:32Z

starter_app/src/app/components/SelectBox.tsx

+            type="text"
+            value={inputDisplayValue}
+            onChange={(e) => setSearchValue(e.target.value)}
+            onClick={(e) => {


If the parent element already handles changing the open state, why is this onClick event needed?

KJES4 · 2025-10-29T14:42:05Z

starter_app/src/app/components/SelectBox.tsx

+              if (!isOpen) setIsOpen(true);
+            }}
+            placeholder={placeholderVisible ? placeholder : ""}
+            readOnly={!search || (!isOpen && !!selectedItem)}


Can you please explain what functionality you are trying to achieve with this conditional statement?

KJES4 · 2025-10-29T14:44:00Z

starter_app/src/app/page.tsx

+                options={selectBoxGroupedOptions}
+                placeholder="Select Properties"
+                search={true}
+                multiSelect={false}


The default value for this prop is false so you don't need to redefine a value of false here as well.

KJES4 · 2025-10-29T14:44:07Z

starter_app/src/app/page.tsx

+        <SelectBox
+              placeholder="Select Properties"
+              options={flatSelectBoxOptions}
+              multiSelect={false}


The default value for this prop is false so you don't need to redefine a value of false here as well.

KJES4 · 2025-10-29T14:54:02Z

starter_app/src/app/components/SelectBox.tsx

+              isOpen ? "rotate-180" : "rotate-0"
+            } ${theme.textOnSurface}`}
+          >
+            <ChevronDown />


This icon isn't showing up in the UI for me.

KJES4 · 2025-10-29T15:35:07Z

starter_app/src/app/components/SelectBox.tsx

+
+      // Focus input when opening and searchable
+      if (!isOpen && search && inputRef.current) {
+        setTimeout(() => inputRef.current?.focus(), 0);


Can you please explain what you are attempting to do with this conditional setTimeout statement?

amnambiar · 2025-10-31T17:38:14Z

Reworking on the PR to be in sync with changes of Dropdown component as in 8230865; updating and reusing Menu

…CHA-react-FE-template into nextjs-component/SelectBox

amnambiar · 2025-11-24T04:53:36Z

@KJES4 , I've updated the PR with a revised SelectBox. Do have a fresh look at it. Let me know if you still see something that needs to be modified.

A very wide summary

Corrected style classes
Menu-based integration
All variant demos on Page
Keyboard support

KJES4 · 2025-11-25T18:19:37Z

starter_app/src/app/components/ControlledTextfield.tsx

            <Button 
              variant="embedded"
              onMouseDown={handleClear} 
-              content={hasError && !isFocused ? {svg: <ExclamationCircleIcon />, mode: 'stroke', size: 'small', strokeWidth: 1.5, color: 'error'} : { svg: <XCircleIcon />, mode: 'stroke', size: 'small', strokeWidth: 1.5, color: 'onVariant' }} />


Please remove the change in this doc. I added the size prop back to the nextjs-tailwind branch. The size prop needs to remain here or else it breaks the styling of the icons.

Understood 👍 Thanks for pointing it! Reverted.

Yes, I think this might have been a mistake on my end. I originally removed these styles then I added them back.

KJES4 · 2025-11-25T18:19:54Z

starter_app/src/app/components/UncontrolledTextfield.tsx

            <Button
              variant="embedded"
              onMouseDown={handleClear}
-              content={hasError && !isFocused ? { svg: <ExclamationCircleIcon />, mode: 'stroke', size: 'small', strokeWidth: 1.5, color: 'error' } : {svg: <XCircleIcon />, mode: 'stroke', size: 'small', strokeWidth: 1.5, color: 'onVariant'}}/>


Please remove the change in this doc. I added the size prop back to the nextjs-tailwind branch. The size prop needs to remain here or else it breaks the styling of the icons.

Understood 👍 Thanks for pointing it! Reverted.

KJES4 · 2025-11-25T18:31:35Z

starter_app/src/app/components/Chip.tsx

 }

-const chipVariants = cva('inline-flex items-center justify-center rounded-md text-xs font-medium whitespace-nowrap p-2 text-onVariant/90 hover:text-onSurface focus-within:ring-1 focus-within:ring-primary', {
+const chipVariants = cva('inline-flex items-center justify-center rounded-md text-xs font-medium whitespace-nowrap p-2 text-onVariant/90 hover:text-onSurface focus-within:ring-1 focus-within:ring-primary ml-1', {


Why did you add the margin-left style to the chip component?

Unnecessary 👍 Removed it. Did add initially to give gap between chips

KJES4 · 2025-11-25T18:35:04Z

starter_app/src/app/components/Menu.tsx

+  selected?: boolean;
+  condensed?: boolean;
+  value?: string;
+  highlighted?: boolean;


What is the difference between selected and highlighted here?

Selected - currently chosen (1st item)
Highlighted - currently focused via keyboard navigation (2nd item)

KJES4 · 2025-12-01T15:32:06Z

starter_app/src/app/components/SelectBox.tsx

+
+        matchedValues = [...matchedValues, ...childValues];
+
+        return cloneElement(child, {


I noticed that cloneElement is used in several places in this file. What rational led you to make this choice?

KJES4 · 2025-12-01T15:46:58Z

starter_app/src/app/components/SelectBox.tsx

+  };
+
+  // Handle keyboard navigation
+  const handleKeyDown = (e: React.KeyboardEvent) => {


Can you explain to me how this keyboard navigation is supposed to work?

Added SelectBox with hierarchical grouped list, simple flat list, sea…

b6800cd

…rch, multiselect(show all options/ x more)

amnambiar changed the base branch from main to nextjs-tailwind October 14, 2025 07:17

amnambiar requested a review from KJES4 October 14, 2025 07:17

Merge branch 'nextjs-tailwind' of https://github.com/input-output-hk/…

3186f05

…CHA-react-FE-template into nextjs-component/SelectBox

KJES4 reviewed Oct 28, 2025

View reviewed changes

KJES4 requested changes Oct 28, 2025

View reviewed changes

KJES4 reviewed Oct 29, 2025

View reviewed changes

KJES4 self-assigned this Nov 5, 2025

amnambiar added 3 commits November 24, 2025 09:46

Modifications to SelectBox architecture

327ffb5

Merge branch 'nextjs-tailwind' of https://github.com/input-output-hk/…

f58cf68

…CHA-react-FE-template into nextjs-component/SelectBox

Added status-icons in grouped-select and other fixes

d0801e1

KJES4 reviewed Nov 25, 2025

View reviewed changes

amnambiar added 2 commits November 26, 2025 11:15

Revert size prop change

10d0db2

Reverted Chip margin

9578312

KJES4 reviewed Dec 1, 2025

View reviewed changes


		matchedValues = [...matchedValues, ...childValues];

		return cloneElement(child, {

[Component] SelectBox #4

Are you sure you want to change the base?

[Component] SelectBox #4

Uh oh!

Conversation

amnambiar commented Oct 14, 2025

Pull request template

Description

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KJES4 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amnambiar commented Oct 31, 2025

Uh oh!

amnambiar commented Nov 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels