Skip to content

Select component#83

Merged
pruthvi2103 merged 13 commits intomainfrom
select-component
Sep 2, 2022
Merged

Select component#83
pruthvi2103 merged 13 commits intomainfrom
select-component

Conversation

@saurabhsutar192
Copy link
Copy Markdown
Contributor

@saurabhsutar192 saurabhsutar192 commented Sep 2, 2022

Select Component

Description

Select component is a combobox that supports selecting or searching and selecting and many more things

Motivation and Context

A versatile select box that support many inbuilt functionalities

How Has This Been Tested?

Manual and Local Build successful

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@saurabhsutar192 saurabhsutar192 added the New Component Addition of new Components label Sep 2, 2022
@saurabhsutar192 saurabhsutar192 self-assigned this Sep 2, 2022
@netlify
Copy link
Copy Markdown

netlify bot commented Sep 2, 2022

Deploy Preview for hover-design canceled.

Name Link
🔨 Latest commit f8152f3
🔍 Latest deploy log https://app.netlify.com/sites/hover-design/deploys/6311dfaeac06e00008cc60d3

Copy link
Copy Markdown
Contributor

@pruthvi2103 pruthvi2103 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments, also while testing i found following problems:

During tab navigation, pressing space does not open the select.

Comment thread lib/src/components/ComboBox/Select.tsx Outdated
SelectPropsType
> = (
{
placeholder = "Pick one",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not keep default placeholders, it results un-intentional behaviour keep it blank

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Comment thread lib/src/components/ComboBox/Select.tsx Outdated
value,
height = "40px",
width = "100%",
roundness = "0",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isnt borderRadius a better name?

Comment thread lib/src/components/ComboBox/Select.tsx Outdated
isMulti = false,
DropIcon,
error = false,
nothingFoundLabel = "Nothing Found!",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again do not add default text here

Comment on lines +53 to +54
onDropDownClose = () => {},
onDropDownOpen = () => {},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: do not add default value here, if its not provided let it be undefined

Comment thread lib/src/index.ts Outdated
export * from "./components/Tab";
export * from "./components/Avatar";
export * from "./components/Table";
export * from "./components/ComboBox";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we decided to change name to Select right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forgot to rename the folder


// Reload only if ref or handler changes
}, [ref, handler]);
}, [ref, handler, isCallbackEnabled]);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice one!

Copy link
Copy Markdown
Contributor

@pruthvi2103 pruthvi2103 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one change

Comment thread lib/src/icons/Icons.tsx Outdated
Comment on lines +1 to +41
export const ArrowDown = () => {
return (
<svg
xmlns="http://www.w3.org/2000/svg"
width={18}
height={18}
viewBox="0 0 24 24"
strokeWidth={2}
stroke="currentColor"
fill="none"
strokeLinecap="round"
strokeLinejoin="round"
>
<path stroke="none" d="M0 0h24v24H0z" fill="none"></path>
<line x1={12} y1={5} x2={12} y2={19}></line>
<line x1={18} y1={13} x2={12} y2={19}></line>
<line x1={6} y1={13} x2={12} y2={19}></line>
</svg>
);
};

export const Clear = () => {
return (
<svg
xmlns="http://www.w3.org/2000/svg"
className="icon icon-tabler icon-tabler-circle-x"
width={18}
height={18}
viewBox="0 0 24 24"
strokeWidth="2"
stroke="currentColor"
fill="none"
strokeLinecap="round"
strokeLinejoin="round"
>
<path stroke="none" d="M0 0h24v24H0z" fill="none"></path>
<circle cx={12} cy={12} r={9}></circle>
<path d="M10 10l4 4m0 -4l-4 4"></path>
</svg>
);
};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shift this to lib/src/components/_internal/Icons

Copy link
Copy Markdown
Contributor

@pruthvi2103 pruthvi2103 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pruthvi2103 pruthvi2103 merged commit a6de6b5 into main Sep 2, 2022
@pruthvi2103 pruthvi2103 mentioned this pull request Sep 2, 2022
3 tasks
@pruthvi2103 pruthvi2103 added this to the v0.1.8-alpha milestone Sep 2, 2022
@pruthvi2103 pruthvi2103 mentioned this pull request Sep 2, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

New Component Addition of new Components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants