Rewrite select #137

vineethasok · 2023-09-12T12:01:39Z

Screen.Recording.2023-09-21.at.08.56.51.mov

vercel · 2023-09-12T12:01:43Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
click-ui	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 26, 2023 1:48pm

vineethasok · 2023-09-14T09:38:56Z

@serdec @fneves I'm facing 2 issues in this PR

The trigger is loaded before the content and the element value is not added on initial render
On Multi Select when we are dragging outside the select trigger and dropping it won't revert the change entirely and move to the previous change. For example if there are 3 elements and the first element is dragged above the second one and then without dropping I drag it outside the trigger element then the second position is added and not reverted to the first position

fneves · 2023-09-15T06:07:35Z

Have a look at this: https://ui.shadcn.com/docs/components/combobox
Its based in radix and I think its what we want.

vineethasok · 2023-09-15T11:05:11Z

@fneves The way they do it is using the cmdk package and we have encountered the problem of case sensitiveness when navigating through options
That's why we are trying to rewrite the select package

serdec · 2023-09-15T15:13:18Z

@serdec @fneves I'm facing 2 issues in this PR

The trigger is loaded before the content and the element value is not added on initial render

On Multi Select when we are dragging outside the select trigger and dropping it won't revert the change entirely and move to the previous change. For example if there are 3 elements and the first element is dragged above the second one and then without dropping I drag it outside the trigger element then the second position is added and not reverted to the first position

@vineethasok where do we use the select with drag and drop at the moment? Maybe if it is giving us too many troubles we can ditch the drag an drop for now and implement a simpler version and refine it later on

serdec · 2023-09-16T05:18:16Z

the documentation is not showing up correctly

serdec · 2023-09-16T05:18:55Z

it seems the showSearch flag doesn't work correctly

serdec · 2023-09-16T05:22:24Z

src/components/MultiSelect/MultiSelect.tsx

+}: MultiSelectProps) => {
+  const defaultId = useId();
+  const inputRef = useRef<HTMLInputElement>(null);
+  const [open, setOpen] = useState(defaultOpen ?? openProp);


i would avoid setting the internal open state as this can lead to inconsistencies such as a situation where the user open state is false but the select is open.

This is added as in radix ui in order to have an option to not declare it externally and we can still have the functionality
In this case the user can define the initial open state and not manage the open state externally and use the select like normally

vineethasok · 2023-09-16T14:58:28Z

@serdec The sort issue is fixed only issue is the first time visibility of the elements
I managed to fix the item loading on initial render issue too
But not confident it is the right way

vineethasok · 2023-09-16T15:02:11Z

src/components/Select/Select.tsx

+const Trigger = forwardRef<HTMLButtonElement, TriggerProps>(
+  ({ placeholder = "Select an option", onClick: onClickProp, ...props }, ref) => {
+    const [, forceUpate] = useState(0);
+    const { disabled, id, hasError, selectedValues, updateSearch, getValueProps } =


@serdec @fneves @gjones Do you think elements such as disabled, id, hasError should be moved to a different context and also I should create a separate component with the useCombobox for the value only?

vineethasok · 2023-09-16T15:03:27Z

src/components/Combobox/ComboBoxElements.tsx

+
+export const ComboboxGroup = forwardRef<HTMLDivElement, GroupProps>(
+  ({ children, heading, ...props }, forwardedRef) => {
+    useCombobox(); // inorder to refresh the content on search


@serdec @fneves @gjones Do you think I should create a hook as below inorder for this component to be rerendered only on search?

import { useMemo } from "react"; import { useCombobox } from "./useCombobox"; const useSearch = () => { const { search } = useCombobox(); const searchValue = useMemo(() => search, [search]); return searchValue; }; export default useSearch;

src/components/Select/common/InternalSelect.tsx

serdec · 2023-09-25T09:36:21Z

src/components/Select/MultiSelect.tsx

+  defaultOpen?: boolean;
+}
+
+export const MultiSelect = ({


options are not aligned if there's an icon

serdec · 2023-09-25T09:54:32Z

src/components/Select/SingleSelect.tsx

+  );
+  const [open, setOpen] = useState(false);
+
+  const onOpenChange = useCallback((newOpen?: boolean) => {


you don't need to pass newOpen

serdec · 2023-09-25T09:55:09Z

src/components/IconWrapper/IconWrapper.tsx

+  height?: number | string;
+}) => {
+  return (
+    <LabelContainer>


label is not aligned when there's an error

This error may happen to the input and other form elements too
Should I fix them in this PR or a new one separately for them? @serdec

another one is ok

serdec · 2023-09-25T09:59:05Z

src/components/Select/SingleSelect.stories.tsx

+        onCreateOption={
+          clickableNoData ? search => console.log("Clicked ", search) : undefined
+        }
+        options={selectOptions}


the options property is not displayed

serdec · 2023-09-25T10:30:11Z

src/components/Select/MultiSelect.tsx

+  const onChange = useCallback(
+    (values: Array<string>) => {
+      setSelectedValues(values);
+      if (typeof onChangeProp === "function") {
+        onChangeProp(values);
+      }
+    },
+    [onChangeProp]
+  );


do we really need this onChange prop? how should the user use it compared to the onSelect?

On Change Passes the entire list
In case on onSelectpeople expect the last clicked element in case of addition of removal
I can add the onSelect if needed

i think the onSelect should pass all the selected values so that the user doesn't have to keep track of the changes and we can get rid of the onChanges prop

Got it will rename onChange with onSelect

serdec · 2023-09-25T14:00:12Z

src/components/Select/SingleSelect.tsx

@@ -26,8 +26,8 @@ export const Select = ({
  );
  const [open, setOpen] = useState(false);

-  const onOpenChange = useCallback((newOpen?: boolean) => {
-    setOpen(open => newOpen ?? !open);
+  const onOpenChange = useCallback((open: boolean) => {


this should be a toggle

We use onOpenChange for the radix-based items like dropdown, context menu , tooltip and all
So in order to have consistency I added onOpenChange instead of onToggle

gjones · 2023-09-25T19:10:21Z

Select component

When disabled, can we add cursor: not-allowed
When show search is enabled, there's an extra horizontal line that I don't think we need.

Multiselect component

I see it (looks great) but I can't interact with it. x doesn't do anything and I can't select new options.

vineethasok · 2023-09-25T20:16:03Z

Select component

When disabled, can we add cursor: not-allowed

When show search is enabled, there's an extra horizontal line that I don't think we need.

### Multiselect component * I see it (looks great) but I can't interact with it. `x` doesn't do anything and I can't select new options.

@gjones
The close is working but when a value is provided then that value is respected over the one which we use inside
Remove the value in stories and try selecting and deleting it
It Will work

Regarding the line
I copied the design from this figma file and it has a line under the select
Should I remove it?
https://www.figma.com/file/mjSci5n5YREyVlvaVjMSDq/Click-UI-Library?type=design&node-id=472%3A6842&mode=design&t=zME5E6bf4uUv5Xbr-1

vineethasok requested review from gjones, fneves and serdec September 12, 2023 12:01

vineethasok self-assigned this Sep 12, 2023

vineethasok marked this pull request as draft September 12, 2023 12:01

vercel bot had a problem deploying to Preview September 12, 2023 12:02 Failure

vercel bot had a problem deploying to Preview September 14, 2023 09:08 Failure

vercel bot had a problem deploying to Preview September 14, 2023 09:09 Failure

vercel bot had a problem deploying to Preview September 14, 2023 15:24 Failure

vineethasok marked this pull request as ready for review September 15, 2023 19:26

vercel bot had a problem deploying to Preview September 15, 2023 19:43 Failure

vercel bot had a problem deploying to Preview September 15, 2023 20:11 Failure

vercel bot had a problem deploying to Preview September 15, 2023 20:13 Failure

vercel bot deployed to Preview September 15, 2023 20:52 View deployment

serdec reviewed Sep 16, 2023

View reviewed changes

vercel bot deployed to Preview September 16, 2023 14:58 View deployment

vineethasok commented Sep 16, 2023

View reviewed changes

vercel bot deployed to Preview September 17, 2023 12:49 View deployment

vercel bot deployed to Preview September 17, 2023 15:46 View deployment

vineethasok force-pushed the rewrite-select branch from 09a7a61 to c1ecdba Compare September 18, 2023 10:34

vercel bot deployed to Preview September 18, 2023 10:35 View deployment

vineethasok added 4 commits September 25, 2023 11:50

Add Select Changes

79d47fc

Fix Multi select sorting

8ce431c

Remove open from story book

7d1adaa

Fix initial selection and also props naming

3037322

vineethasok force-pushed the rewrite-select branch from 103e821 to 3037322 Compare September 25, 2023 09:51

vercel bot deployed to Preview September 25, 2023 09:52 View deployment

serdec reviewed Sep 25, 2023

View reviewed changes

Add styling changes

c9fd4b6

vercel bot deployed to Preview September 25, 2023 12:42 View deployment

vineethasok added 3 commits September 25, 2023 14:55

Change onChange to onSelect in Select and MultiSelect props

a0e5ec6

Fix Select Test

f3c7286

Add types

40a0950

vercel bot deployed to Preview September 25, 2023 13:00 View deployment

Fix incorrect assignment

55a48a2

vercel bot deployed to Preview September 25, 2023 13:04 View deployment

vineethasok requested a review from serdec September 25, 2023 13:58

serdec reviewed Sep 25, 2023

View reviewed changes

serdec approved these changes Sep 25, 2023

View reviewed changes

Fix Select Styling

626241e

vercel bot deployed to Preview September 25, 2023 20:46 View deployment

Merge branch 'main' into rewrite-select

c487ce0

vercel bot deployed to Preview September 25, 2023 20:53 View deployment

vineethasok and others added 2 commits September 26, 2023 15:25

Merge branch 'main' into rewrite-select

721b8ac

Add Ellipsis changes

85460a1

vercel bot deployed to Preview September 26, 2023 13:48 View deployment

vineethasok merged commit b3fa8d6 into main Sep 26, 2023

vineethasok deleted the rewrite-select branch September 26, 2023 18:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite select #137

Rewrite select #137

vineethasok commented Sep 12, 2023 •

edited

Loading

vercel bot commented Sep 12, 2023 •

edited

Loading

vineethasok commented Sep 14, 2023

fneves commented Sep 15, 2023

vineethasok commented Sep 15, 2023

serdec commented Sep 15, 2023

serdec commented Sep 16, 2023

serdec commented Sep 16, 2023

serdec Sep 16, 2023

vineethasok Sep 16, 2023

vineethasok commented Sep 16, 2023

vineethasok Sep 16, 2023

vineethasok Sep 16, 2023 •

edited

Loading

serdec Sep 25, 2023

serdec Sep 25, 2023

serdec Sep 25, 2023

vineethasok Sep 25, 2023

serdec Sep 25, 2023

serdec Sep 25, 2023

serdec Sep 25, 2023

vineethasok Sep 25, 2023

serdec Sep 25, 2023

vineethasok Sep 25, 2023

serdec Sep 25, 2023

vineethasok Sep 25, 2023

gjones commented Sep 25, 2023

vineethasok commented Sep 25, 2023

Select component

Rewrite select #137

Rewrite select #137

Conversation

vineethasok commented Sep 12, 2023 • edited Loading

vercel bot commented Sep 12, 2023 • edited Loading

vineethasok commented Sep 14, 2023

fneves commented Sep 15, 2023

vineethasok commented Sep 15, 2023

serdec commented Sep 15, 2023

serdec commented Sep 16, 2023

serdec commented Sep 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vineethasok commented Sep 16, 2023

Choose a reason for hiding this comment

vineethasok Sep 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gjones commented Sep 25, 2023

Select component

Multiselect component

vineethasok commented Sep 25, 2023

Select component

vineethasok commented Sep 12, 2023 •

edited

Loading

vercel bot commented Sep 12, 2023 •

edited

Loading

vineethasok Sep 16, 2023 •

edited

Loading