try to customise multi select dropdown component with Ids #187

kevinleeTCA · 2025-01-09T02:57:59Z

create a custom component based on the existing one, then during the loadData step I'd get both names and IDs and combine them into a single object, then use that in index.ts to do the rendering and set the option values with the ID.

kevinleeTCA · 2025-01-09T02:59:48Z

src/components/vanilla/controls/MultiSelectDropdownWithIds/MultiSelectDropdownWithIds.emb.ts

+    {
+      name: 'id',
+      type: 'dimension',
+      label: 'ID',
+      config: {
+        dataset: 'ds'
+      },
+      category: 'Dropdown ids'
+    },


This is where we add the id dimension that later will be linked to a variable to filter other charts data using id (built in index)

kevinleeTCA · 2025-01-09T03:02:06Z

src/components/vanilla/controls/MultiSelectDropdownWithIds/MultiSelectDropdownWithIds.emb.ts

+    // {
+    //   name: 'defaultValue',
+    //   type: 'string',
+    //   array: true,
+    //   label: 'Default value',
+    //   category: 'Pre-configured variables'
+    // },


Temporarily comment this out due to a type inconsistent issue.

kevinleeTCA · 2025-01-09T03:03:12Z

src/components/vanilla/controls/MultiSelectDropdownWithIds/MultiSelectDropdownWithIds.emb.ts

+      ...inputs,
+      options: loadData({
+        from: inputs.ds,
+        dimensions: inputs.property ? [inputs.property, inputs.id] : [],


This is where we can query the id field when doing the dropdown search:

kevinleeTCA · 2025-01-09T03:04:04Z

src/components/vanilla/controls/MultiSelectDropdownWithIds/MultiSelectDropdownWithIds.emb.ts

+      return {
+        value: value.length ? value : Value.noFilter(),
+        id: id.length ? id : Value.noFilter()
+      };


This is where we would like to capture an object (instead of value string) to be able to assign the id to a variable used for filters

kevinleeTCA · 2025-01-09T03:04:49Z

src/components/vanilla/controls/MultiSelectDropdownWithIds/index.tsx

+export type EventPayload = {
+    value: string;
+    id: string;
+}


Now the event payload should have an object type instead of string

kevinleeTCA · 2025-01-09T03:05:18Z

src/components/vanilla/controls/MultiSelectDropdownWithIds/index.tsx

+  className?: string;
+  options: DataResponse;
+  unclearable?: boolean;
+  onChange: (v: EventPayload[]) => void;


and onChange should capture event payload containing both value and id

kevinleeTCA · 2025-01-09T03:06:34Z

src/components/vanilla/controls/MultiSelectDropdownWithIds/index.tsx

+  const set = useCallback(
+    (newValue: EventPayload) => {
+      performSearch('');
+
+      let newValues: EventPayload[] = [];
+
+      if (newValue.value !== '') {
+        newValues = value || [];
+        if (newValues?.includes(newValue)) {
+          newValues = newValues.filter((v) => v.value !== newValue.value);
+        } else {
+          newValues = [...newValues, newValue];
+        }
+      }
+
+      props.onChange(newValues);
+      setValue(newValues);
+      setServerSearch((s) => ({ ...s, [props.searchProperty || 'search']: '' }));
+      clearTimeout(debounce);
+    },


This is the main change in the index to support an object set from old to new. But I am still encountering issues in the preview mode when select any item from dropdown:

…anilla-components into multiSelectDropdownWithIds

greptile-apps

PR Summary

This PR introduces a new MultiSelectDropdownWithIds component that extends the existing dropdown functionality to support both display values and corresponding IDs, enabling more complex data relationships in selections.

The EventPayload type in /src/components/vanilla/controls/MultiSelectDropdownWithIds/index.tsx needs proper handling in the set() function for ID field management
Global debounce variable in /src/components/vanilla/controls/MultiSelectDropdownWithIds/index.tsx should be moved into component scope to prevent cross-instance interference
The includes() check in the list rendering logic needs to be updated to properly compare objects with both value and ID fields
The component adds new id dimension input in .emb.ts to load corresponding IDs alongside property values

_{💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!}

_{2 file(s) reviewed, 5 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2025-02-02T00:15:37Z

src/components/vanilla/controls/MultiSelectDropdownWithIds/MultiSelectDropdownWithIds.emb.ts

+      ...inputs,
+      options: loadData({
+        from: inputs.ds,
+        dimensions: inputs.property ? [inputs.property, inputs.id] : [],


logic: No check for inputs.id before including it in dimensions array. Could cause runtime issues if id field not configured.

Suggested change

dimensions: inputs.property ? [inputs.property, inputs.id] : [],

dimensions: inputs.property && inputs.id ? [inputs.property, inputs.id] : [],

greptile-apps · 2025-02-02T00:15:38Z

src/components/vanilla/controls/MultiSelectDropdownWithIds/MultiSelectDropdownWithIds.emb.ts

+      options: loadData({
+        from: inputs.ds,
+        dimensions: inputs.property ? [inputs.property, inputs.id] : [],
+        limit: inputs.limit || 1000,


style: Default limit of 1000 differs from meta default of 100. Should be consistent.

greptile-apps · 2025-02-02T00:15:45Z

src/components/vanilla/controls/MultiSelectDropdownWithIds/index.tsx

+        if (newValues?.includes(newValue)) {
+          newValues = newValues.filter((v) => v.value !== newValue.value);


logic: Array.includes() won't work correctly for object comparison. Need to use array.find() with proper value/id comparison

Suggested change

if (newValues?.includes(newValue)) {

newValues = newValues.filter((v) => v.value !== newValue.value);

if (newValues?.find(v => v.value === newValue.value && v.id === newValue.id)) {

newValues = newValues.filter((v) => v.value !== newValue.value);

greptile-apps · 2025-02-02T00:15:45Z

src/components/vanilla/controls/MultiSelectDropdownWithIds/index.tsx

+            key={i}
+            onClick={() => {
+              setTriggerBlur(false);
+              set(o[props.property?.name || ''] || '');


logic: Missing ID field in set() call. Should be set({value: o[props.property?.name || ''], id: o.id}) to properly handle both value and ID

Suggested change

set(o[props.property?.name || ''] || '');

set({value: o[props.property?.name || ''] || '', id: o.id});

greptile-apps · 2025-02-02T00:15:46Z

src/components/vanilla/controls/MultiSelectDropdownWithIds/index.tsx

+
+type Record = { [p: string]: string };
+
+let debounce: number | undefined = undefined;


style: Global debounce variable will cause issues with multiple component instances. Move inside component using useRef

Suggested change

let debounce: number | undefined = undefined;

const debounce = useRef<number>();

try to customise multi select dropdown component with Ids

bdf7866

kevinleeTCA commented Jan 9, 2025

View reviewed changes

kevinleeTCA added 2 commits January 15, 2025 15:53

multi select dropdown with ids

2e31323

Merge branch 'multiSelectDropdownWithIds' of github.com:kevinleeTCA/v…

b0d0012

…anilla-components into multiSelectDropdownWithIds

kevinleeTCA mentioned this pull request Jan 15, 2025

Multi select dropdown with ids ailohq/vanilla-components#1

Closed

greptile-apps bot reviewed Feb 2, 2025

View reviewed changes

	dimensions: inputs.property ? [inputs.property, inputs.id] : [],
	dimensions: inputs.property && inputs.id ? [inputs.property, inputs.id] : [],

		if (newValues?.includes(newValue)) {
		newValues = newValues.filter((v) => v.value !== newValue.value);

	set(o[props.property?.name \|\| ''] \|\| '');
	set({value: o[props.property?.name \|\| ''] \|\| '', id: o.id});


		type Record = { [p: string]: string };

		let debounce: number \| undefined = undefined;

	let debounce: number \| undefined = undefined;
	const debounce = useRef<number>();

try to customise multi select dropdown component with Ids #187

Are you sure you want to change the base?

try to customise multi select dropdown component with Ids #187

Uh oh!

Conversation

kevinleeTCA commented Jan 9, 2025

Uh oh!

kevinleeTCA Jan 9, 2025

Choose a reason for hiding this comment

Uh oh!

kevinleeTCA Jan 9, 2025

Choose a reason for hiding this comment

Uh oh!

kevinleeTCA Jan 9, 2025

Choose a reason for hiding this comment

Uh oh!

kevinleeTCA Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinleeTCA Jan 9, 2025

Choose a reason for hiding this comment

Uh oh!

kevinleeTCA Jan 9, 2025

Choose a reason for hiding this comment

Uh oh!

kevinleeTCA Jan 9, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

Uh oh!

greptile-apps bot Feb 2, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Feb 2, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Feb 2, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Feb 2, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Feb 2, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

kevinleeTCA Jan 9, 2025 •

edited

Loading