Add new Select component #219

williamkolmacka · 2018-06-19T07:45:50Z

This Pull Request meets the following criteria:

Tests have been added/adjusted for my new feature
New Components are registered in index.js of my project

Add a new Select component. There are still some values in CSS that are not in tokens.

vepor

vepor · 2018-06-19T08:27:33Z

src/Select/Select.js

+            <ChevronDown />
+          </SelectSuffix>
+        </SelectContainer>
+        {help ? <Feedback type="help">{help}</Feedback> : <Feedback type="error">{error}</Feedback>}


Evaluate if help or error is defined. It renders empty component now.

vepor · 2018-06-19T08:49:38Z

src/Select/Select.js

+  pointer-events: none;
+`;
+
+const SelectSizeCondition = (theme, size) => {


I would prefer that you will use this condition directly on line 88 and delete this const. Also please add TODO for token create.

vepor · 2018-06-19T09:14:07Z

src/Select/Select.js

+    this.props.onChange(e);
+  };
+
+  renderOption = ({ value, label, visible = true }: Option) => {


You defined disabled in Option type but you are not rendering it anywhere.

vepor · 2018-06-19T09:26:53Z

src/Select/Select.js

+    cursor: default;
+  }
+  /* placeholder */
+  &:invalid {


Please for placeholder style use another way. Required attr will probably render native browser error. Do this like:

const isPlaceholder = this.props.placeholder && this.state.value === ""

You then pass this const to StyledSelect and it'll decide which color should apply.

vepor · 2018-06-19T09:28:17Z

src/Select/Select.js

+  value: string | number,
+  label?: string,
+  disabled?: boolean,
+  visible?: boolean, // eslint-disable-line react/no-unused-prop-types


The is no logical reason to use visible more, please delete it everywhere.

vepor · 2018-06-19T09:36:39Z

src/Select/Select.js

+    const { value } = this.state;
+    return (
+      <React.Fragment>
+        {label && <Text>{label}</Text>}


I'm completely missing label knob in stories. Don't forget that label's color is changing on the selected value.

vepor · 2018-06-19T09:39:30Z

src/Select/__tests__/Select.test.js

 describe("Select", () => {
  const component = shallow(<Select value="1" options={objectOptions} onChange={mockChange} />);
+  const componentWithPlaceholder = shallow(


You can merge these two shallow wrappers into one.

vepor · 2018-06-19T13:08:25Z

src/Select/Select.js

+  margin-bottom: ${({ theme }) => theme.spaceXXSmall};
+`;
+
+const SelectContainer = styled(({ theme, ...props }) => <div {...props} />)`


Omit error or:

const SelectContainer = styled(({ children, className }) => ( <div className={className}>{children}</div> ))`

vepor · 2018-06-19T13:12:47Z

src/Select/Select.js

+  box-sizing: border-box;
+  cursor: pointer;
+  &:hover {
+    border-color: ${({ theme, active, error }) =>


Disable hover when select is disabled

vepor · 2018-06-19T13:16:42Z

src/Select/Select.js

+    return (
+      <React.Fragment>
+        {label && (
+          <SelectLabel theme={theme} value={value}>


Should be select focused and opened after clicking on label?

Can make this when we will have custom select ready. For now, it is not so much important.

vepor

And don't forget to rebase 🙏

vepor · 2018-06-20T10:48:28Z

src/Select/Select.js

+  label?: string,
+  placeholder?: string,
+  value: string,
+  disabled?: boolean,


When select is disabled, styles are not changed.

Good catch 👏

vepor · 2018-06-20T10:50:03Z

src/Select/Select.stories.js

+    const placeholder = text("Placeholder", "Select value from list");
+    const size = select("Size", Object.values(SIZE_OPTIONS), SIZE_OPTIONS.NORMAL);
+    const disabled = boolean("Disabled", false);
+    const option = object("Options", objectOptions);


I would add value knob to playground.

vepor · 2018-06-20T10:52:58Z

src/Select/__tests__/Select.test.js

@@ -5,6 +5,8 @@ import { shallow } from "enzyme";

 import Select from "../Select";

+const mockChange = jest.fn();
+const placeholder = "Default placeholder";
 const objectOptions = [


visible is not needed in this object more.

vepor · 2018-06-20T10:55:53Z

src/Select/Select.js

+  padding-left: ${({ prefix }) => prefix && "48px"}; // TODO: Create token
+
+  /* Based on state of select */
+  border-color: ${({ theme, error }) =>


border: 1px solid ....

and delete line 44

It defines basic style of border, so I want to let it

vepor · 2018-06-20T10:56:44Z

src/Select/Select.js

+  }
+`;
+
+const SelectContainer = styled.div`


disabled attr is render for this div

vepor · 2018-06-20T10:56:52Z

src/Select/Select.js

+  pointer-events: none;
+`;
+
+const SelectSuffix = styled.div`


disabled attr is render for this div

vepor

Good job 👏

williamkolmacka added the Review needed Pull Requests that are considered complete label Jun 19, 2018

williamkolmacka added this to the Orbit Components 0.2 milestone Jun 19, 2018

williamkolmacka self-assigned this Jun 19, 2018

williamkolmacka requested a review from tomashapl June 19, 2018 07:45

williamkolmacka requested a review from jaroslav-kubicek as a code owner June 19, 2018 07:45

williamkolmacka force-pushed the new-select branch from cd8b05f to a009d91 Compare June 19, 2018 07:51

tomashapl requested a review from vepor June 19, 2018 08:14

vepor suggested changes Jun 19, 2018

View reviewed changes

vepor removed the Review needed Pull Requests that are considered complete label Jun 19, 2018

tomashapl modified the milestones: Orbit Components 0.2, Orbit Components 0.1 Jun 19, 2018

williamkolmacka force-pushed the new-select branch 2 times, most recently from ee3fcb1 to 3ee0295 Compare June 19, 2018 12:49

vepor suggested changes Jun 19, 2018

View reviewed changes

williamkolmacka force-pushed the new-select branch from 3ee0295 to f27fd45 Compare June 19, 2018 13:58

vepor added the Review needed Pull Requests that are considered complete label Jun 19, 2018

tomashapl force-pushed the new-select branch from f27fd45 to ab09563 Compare June 20, 2018 10:13

vepor suggested changes Jun 20, 2018

View reviewed changes

tomashapl force-pushed the new-select branch 3 times, most recently from 13afa56 to 7a7613e Compare June 20, 2018 12:20

vepor previously approved these changes Jun 20, 2018

View reviewed changes

NEW: Select

1afc914

tomashapl dismissed vepor’s stale review via 1afc914 June 20, 2018 12:23

tomashapl force-pushed the new-select branch from 7a7613e to 1afc914 Compare June 20, 2018 12:23

vepor approved these changes Jun 20, 2018

View reviewed changes

vepor removed the Review needed Pull Requests that are considered complete label Jun 20, 2018

tomashapl merged commit 3a091ec into master Jun 20, 2018

tomashapl deleted the new-select branch June 20, 2018 12:28

tomashapl mentioned this pull request Jun 25, 2018

Move Label component out of Input folder and let it accept for Typography "types" #66

Closed

4 tasks

vepor mentioned this pull request Jul 10, 2018

Create <Select /> component #159

Closed

vepor pushed a commit that referenced this pull request Aug 27, 2020

chore: bump eslint-plugin-import from 2.20.1 to 2.20.2 (#219)

4e3af6a

vepor pushed a commit that referenced this pull request Nov 4, 2020

chore: bump eslint-plugin-import from 2.20.1 to 2.20.2 (#219)

46605ee

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new Select component #219

Add new Select component #219

williamkolmacka commented Jun 19, 2018

vepor left a comment

vepor Jun 19, 2018

vepor Jun 19, 2018 •

edited

vepor Jun 19, 2018

vepor Jun 19, 2018

vepor Jun 19, 2018

vepor Jun 19, 2018

vepor Jun 19, 2018

vepor Jun 19, 2018

vepor Jun 19, 2018

vepor Jun 19, 2018

williamkolmacka Jun 19, 2018

vepor left a comment

vepor Jun 20, 2018

tomashapl Jun 20, 2018

vepor Jun 20, 2018

tomashapl Jun 20, 2018

vepor Jun 20, 2018

tomashapl Jun 20, 2018

vepor Jun 20, 2018 •

edited

tomashapl Jun 20, 2018

vepor Jun 20, 2018

vepor Jun 20, 2018

tomashapl Jun 20, 2018

vepor Jun 20, 2018

vepor left a comment

Add new Select component #219

Add new Select component #219

Conversation

williamkolmacka commented Jun 19, 2018

vepor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vepor Jun 19, 2018 • edited

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

vepor left a 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

vepor Jun 20, 2018 • edited

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

vepor left a comment

Choose a reason for hiding this comment

vepor Jun 19, 2018 •

edited

vepor Jun 20, 2018 •

edited