Framework - Explicit dropdown groups + smaller fixes #861

Anders2303 · 2025-02-03T15:59:31Z

Fixes some issues in the Dropdown framework component:

Replaces the "group" field on dropdown options, and introduces a new DropdownOptionGroup type to make grouping more explicit
- This resolves Make Dropdown grouping more explicit #816
Allows you to select options on arrow-key press even if the dropdown is closed
Adds a max-size prop to the dropdown rectangle, so it doesnt get too wide
Adds truncation to the dropdown list and input field to adhere to the max-width
Fixed an issue in the textSize util
Made the dropdown list use proper html tags: <ul> and <li>
Fixed the vertical alignment of option adornments

* Made text truncate * Made options use <ul> and <li> tags * Set a max-width option to avoid that the rect gets too large

jorgenherje

Looks very good! Just some minor comments from my side.

frontend/src/lib/components/Dropdown/dropdown.tsx

rubenthoms

Looks good 👍 Some comments below.

frontend/src/lib/components/Dropdown/dropdown.tsx

frontend/src/lib/components/Input/input.tsx

…pover is closed

jorgenherje

Looks good to me! 👍

rubenthoms

The changes look really good 👍 A few comments

rubenthoms · 2025-02-07T08:55:12Z

frontend/src/lib/components/Dropdown/dropdown.tsx

@@ -59,74 +59,101 @@ type DropdownRect = {

 type OptionItem<TValue> = {
    type: "option";
-    parentGroup?: string;
+    // refference to group. Used to get the icon and adding some styling


rubenthoms · 2025-02-07T08:56:17Z

frontend/src/lib/components/Dropdown/dropdown.tsx

 } & DropdownOption<TValue>;

-type SeperatorItem<TValue> = {
-    type: "separator";
+type GroupTitle<TValue> = {


To be consistent, consider calling this GroupTitleItem, or OptionItem just Option.

rubenthoms · 2025-02-07T09:14:04Z

frontend/src/lib/components/Dropdown/dropdown.tsx

+    if (itemList[adjustedIndex].type !== "option") {
+        if (adjustedIndex === 0) return 1;
+
+        adjustedIndex += direction;
+        adjustedIndex = _.clamp(adjustedIndex, 0, itemList.length - 1);
+    }
+
+    if (itemList[adjustedIndex].type !== "option") {
+        throw new Error("Expected option item to follow non-option item!");
+    }


I fear this could become a pitfall as two groups following each other should preferably not cause a crash. Also, the early return in line 125 is dodging the whole check - what if there are two groups in a row at the start of the dropdown in a certain case of auto-generated options where the implementation on the consumer side is lacking functionality for filtering out empty groups? You could consider using the lodash findIndex method as it has an optional fromIndex parameter - thus, you would look for the next valid option from the current index. If -1 is returned, you can start a new search from 0. With direction === -1 you could traverse a reversed array and just adjust the index afterwards (or have a utility function doing the search and just adjust the arguments and return value accordingly).

rubenthoms · 2025-02-07T09:17:35Z

frontend/src/lib/components/Dropdown/dropdown.tsx


    const valueWithDefault = props.value ?? null;
    const [prevValue, setPrevValue] = React.useState<TValue | null>(props.value ?? null);

    const [filter, setFilter] = React.useState<string | null>(null);
    const [prevFilter, setPrevFilter] = React.useState<string | null>(filter);

-    const allOptionsWithSeperators = React.useMemo(() => makeOptionListItems(props.options), [props.options]);
+    const allOptionsWithSeparators = React.useMemo(


Consider renaming this variable as you retired the separator naming scheme.

rubenthoms · 2025-02-07T09:40:08Z

frontend/src/lib/components/Dropdown/dropdown.tsx

-        function handleOptionsChangeEffect() {
-            function handleMouseDown(event: MouseEvent) {
+        function setupInputBlurListener() {
+            // ! The ref is *actually* the div wrapping the input, so normal "blur" is not available


There is an inputRef prop that is referencing the input element. You can add a ref to that to gain control over blur etc. However, I double-checked and there was something messed up with the references - I created a PR to fix the issues: #865

rubenthoms · 2025-02-07T09:41:23Z

frontend/src/lib/components/Dropdown/dropdown.tsx

@@ -59,74 +59,101 @@ type DropdownRect = {

 type OptionItem<TValue> = {
    type: "option";


Consider using an enum as a type here as it is easier to both change/maintain the code and to explicitly find all references.

rubenthoms · 2025-02-07T09:42:31Z

frontend/src/lib/components/Dropdown/dropdown.tsx


-            return () => {
-                document.removeEventListener("mousedown", handleMouseDown);
+            return function removeInputBlurListener() {


removeInputFocusOutListener?

rubenthoms · 2025-02-07T09:43:04Z

frontend/src/lib/components/Dropdown/dropdown.tsx

@@ -182,36 +217,36 @@ export function Dropdown<TValue = string>(props: DropdownProps<TValue>) {
    }, []);

    React.useEffect(
-        function handleOptionsChangeEffect() {
-            function handleMouseDown(event: MouseEvent) {
+        function setupInputBlurListener() {


...focusOut...?

rubenthoms · 2025-02-07T09:43:50Z

frontend/src/lib/components/Dropdown/dropdown.tsx

+                // Close the dropdown
+                if (dropdownVisible && e.key === "Escape") {
+                    // FIXME: the ref is a div, so blur() isn't possible
+                    // inputRef.current?.blur();
+                    closePopover();


Anders2303 added 3 commits February 3, 2025 14:45

Added DropdownOptionGroup type to make grouping more explicit

b8f6e06

Tweaked dropdown rect:

4582834

* Made text truncate * Made options use <ul> and <li> tags * Set a max-width option to avoid that the rect gets too large

Fixed adornment alignments for input

ccc4d6e

Anders2303 requested review from rubenthoms and jorgenherje February 3, 2025 15:59

jorgenherje requested changes Feb 4, 2025

View reviewed changes

rubenthoms requested changes Feb 4, 2025

View reviewed changes

Anders2303 added 3 commits February 5, 2025 11:00

Resolved feedback

5c39f6e

Fixed some minor styling issues

1a34553

Made it possible to change selection with up/down arrows even when po…

dc8aade

…pover is closed

Anders2303 requested review from jorgenherje and rubenthoms February 5, 2025 14:57

jorgenherje approved these changes Feb 6, 2025

View reviewed changes

rubenthoms requested changes Feb 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Framework - Explicit dropdown groups + smaller fixes #861

Framework - Explicit dropdown groups + smaller fixes #861

Anders2303 commented Feb 3, 2025 •

edited

Loading

jorgenherje left a comment

rubenthoms left a comment

jorgenherje left a comment

rubenthoms left a comment

rubenthoms Feb 7, 2025

rubenthoms Feb 7, 2025 •

edited

Loading

rubenthoms Feb 7, 2025 •

edited

Loading

rubenthoms Feb 7, 2025

rubenthoms Feb 7, 2025

rubenthoms Feb 7, 2025

rubenthoms Feb 7, 2025

rubenthoms Feb 7, 2025

rubenthoms Feb 7, 2025

		@@ -59,74 +59,101 @@ type DropdownRect = {

		type OptionItem<TValue> = {
		type: "option";

Framework - Explicit dropdown groups + smaller fixes #861

Are you sure you want to change the base?

Framework - Explicit dropdown groups + smaller fixes #861

Conversation

Anders2303 commented Feb 3, 2025 • edited Loading

jorgenherje left a comment

Choose a reason for hiding this comment

rubenthoms left a comment

Choose a reason for hiding this comment

jorgenherje left a comment

Choose a reason for hiding this comment

rubenthoms left a comment

Choose a reason for hiding this comment

rubenthoms Feb 7, 2025

Choose a reason for hiding this comment

rubenthoms Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

rubenthoms Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

rubenthoms Feb 7, 2025

Choose a reason for hiding this comment

rubenthoms Feb 7, 2025

Choose a reason for hiding this comment

rubenthoms Feb 7, 2025

Choose a reason for hiding this comment

rubenthoms Feb 7, 2025

Choose a reason for hiding this comment

rubenthoms Feb 7, 2025

Choose a reason for hiding this comment

rubenthoms Feb 7, 2025

Choose a reason for hiding this comment

Anders2303 commented Feb 3, 2025 •

edited

Loading

rubenthoms Feb 7, 2025 •

edited

Loading

rubenthoms Feb 7, 2025 •

edited

Loading