Dropdown and Checkbox component #49

AlleeX · 2017-03-24T11:15:20Z

WHAT DOES THIS PR DO:

Adds the DropDown and Checkbox component to Travix UI Kit.

WHERE SHOULD THE REVIEWER START:

Diffs

UNIT AND/OR FUNCTIONAL TESTS:

Adds Unit tests for DropDown and Checkbox component

asci · 2017-04-03T07:34:04Z

components/checkbox/checkbox.js

+Checkbox.defaultProps = {
+  checked: false,
+  disabled: false,
+  inputAttr: {},


potential reference issue, see: #45 (comment)

asci · 2017-04-03T07:36:13Z

components/checkbox/checkbox.js

+  checked,
+  children,
+  disabled,
+  inputAttr,


any use case for inputAttr and labelAttr?

asci · 2017-04-03T08:07:15Z

components/dropDown/dropDown.scss

+    color: $tx-dropdown-filter-select-placeholder-color;
+    font-size: 13px;
+    font-weight: bold;
+    line-height: 28px; // should equal to .Select-control, .Select-input height


why not to use variable then?

asci · 2017-04-03T08:07:50Z

components/dropDown/dropDown.scss

+    }
+  }
+
+  &.cars-dropdown .Select-arrow-zone {


cars this should not be here

oops) Thanks for catch

asci · 2017-04-03T08:10:52Z

components/dropDown/dropdownFilterOptionComponent.js

+import Checkbox from '../checkbox/checkbox';
+
+  /**
+   * @property {Function} onSelect Passed by react-select internally


would be also nice to have some description what is this component about

asci · 2017-04-03T08:12:47Z

components/dropDown/dropdownFilterOptionComponent.js

+    disabled: PropTypes.bool,
+    label: PropTypes.oneOfType([PropTypes.string, PropTypes.number]).isRequired,
+    value: PropTypes.oneOfType([PropTypes.string, PropTypes.number]).isRequired,
+  }),


should be isRequired

asci · 2017-04-03T08:17:03Z

themes/_default.yaml

+    background-color: *blank
+    border-color: *secondary
+    hover:
+      color: *twitter


semantically it is not correct. did you check it against other brands?

asci · 2017-04-03T08:47:35Z

vendors/react-select/control.scss

+		display: inline-block;
+		font-family: inherit;
+		font-size: inherit;
+		// height: $select-input-internal-height;


asci · 2017-04-03T08:49:09Z

vendors/react-select/components.scss

@@ -0,0 +1,5 @@
+@import "control";


is it possible to use react-select as npm module?

coveralls · 2017-04-03T12:53:34Z

Coverage remained the same at 100.0% when pulling c006d59 on AlleeX:dropdown into 3428b75 on Travix-International:master.

coveralls · 2017-04-03T13:12:14Z

Coverage remained the same at 100.0% when pulling b38a9f6 on AlleeX:dropdown into 3428b75 on Travix-International:master.

EduardTrutsyk · 2017-04-03T14:22:09Z

components/checkbox/checkbox.js

+  /**
+   * Represents the callback for onChange event.
+   */
+  onChange: PropTypes.func.isRequired,


why is it required?

@EduardTrutsyk good question. I am assuming it's because this component is a controlled component and therefore it requires an onChange definition (more at: https://facebook.github.io/react/docs/forms.html )

However, I do agree that either we decide to not have a controlled component (therefore the style and value will come from the normal checked attribute on the checkbox, or we go for this approach and then we'll need state, since you need to change the checked attribute (since you can't mutate props nor change them from the inside).

EduardTrutsyk · 2017-04-03T14:27:42Z

components/dropDown/dropDown.js

+
+      optionClassName && classes.push(optionClassName);
+      isSelected && classes.push('is-selected');
+      isFocused && classes.push('is-focused');


now, you can use object instead of array, to apply classes based on props:

const mods = { 'is-focused': isFocused, 'is-selecte': isSelected }; <Option classNames={getClassNamesWithMods('Select-option', mods)} />

I need class names without concatenation

EduardTrutsyk · 2017-04-03T14:29:19Z

components/dropDown/dropDown.js

+    const isFiltermode = this.props.filterMode;
+    const mods = [];
+    isFiltermode && mods.push('filter');
+    isFiltermode && this.props.options.some(option => option.checked) && mods.push('state-active');


EduardTrutsyk · 2017-04-03T14:31:45Z

components/dropDown/dropDown.js

+  /**
+ * Initial field value
+ */
+  value: PropTypes.oneOfType([PropTypes.string, PropTypes.number, PropTypes.array, PropTypes.object]),


PropTypes.any?

EduardTrutsyk · 2017-04-03T14:34:16Z

components/dropDown/dropDown.scss

+    padding: 10px 22px;
+
+    &:first-child {
+      &:after, &:before {


Better to use new line

&:after, &:before {

EduardTrutsyk · 2017-04-03T14:38:35Z

components/index.scss

 @import "./global/global.scss";
 @import "./collapse/collapseItem.scss";
 @import "./list/list.scss";
 @import "./modal/modal.scss";
 @import "./price/price.scss";
 @import "./radioButton/radioButton.scss";
 @import "./spinner/spinner.scss";
+@import "~react-select/scss/default.scss";


I guess, external styles better to include in the top of file, isn't it?

Need for !default;

coveralls · 2017-04-04T06:54:20Z

Coverage remained the same at 100.0% when pulling 55a458b on AlleeX:dropdown into 3428b75 on Travix-International:master.

mAiNiNfEcTiOn · 2017-04-04T08:11:27Z

components/checkbox/checkbox.js

+  return (
+    <label className={className} htmlFor={name}>
+      <input
+        aria-checked={checked}


lovely ❤️

mAiNiNfEcTiOn · 2017-04-04T08:14:13Z

components/checkbox/checkbox.js

+        role="radio"
+        type="checkbox"
+      />
+      <span className="ui-checkbox__text" />


Why do you have a closed <span>? Shouldn't it wrap the {children}?

mAiNiNfEcTiOn · 2017-04-04T08:18:24Z

components/checkbox/checkbox.js

+  /**
+   * Represents the callback for onChange event.
+   */
+  onChange: PropTypes.func.isRequired,


@EduardTrutsyk good question. I am assuming it's because this component is a controlled component and therefore it requires an onChange definition (more at: https://facebook.github.io/react/docs/forms.html )

However, I do agree that either we decide to not have a controlled component (therefore the style and value will come from the normal checked attribute on the checkbox, or we go for this approach and then we'll need state, since you need to change the checked attribute (since you can't mutate props nor change them from the inside).

mAiNiNfEcTiOn · 2017-04-04T08:22:32Z

components/dropDown/dropDown.js

+    this.menuRenderer = this.menuRenderer.bind(this);
+  }
+
+  optionRef = (onOptionRef, isSelected) => ref => onOptionRef(ref, isSelected);


Can you change it to a simple method instead of an arrow function? You're returning another function it gets complicated to read:

static optionRef(onOptionRef, isSelected) { return ref => onOptionRef(ref, isSelected); },

And then change its usage to: Dropdown.optionRef(), since it doesn't require to be bound to this.

coveralls · 2017-04-04T12:35:39Z

Coverage remained the same at 100.0% when pulling d68bebf on AlleeX:dropdown into 3428b75 on Travix-International:master.

AlleeX and others added 21 commits March 16, 2017 16:10

init dropdown component

bfe3b4e

test commit

66f823e

test commit2

68a244c

add react select

d0e4ec8

Merge branch 'dropdown' of github.com:AlleeX/travix-ui-kit into dropdown

a2e2a6f

add styles and examples for dropdown

5a3e147

added initial unit tests for dropdown

695375b

Merge branch 'master' into dropdown

f1d72eb

improve styles for dd

913bd6c

add checkbox

e75300a

add filter mode for dropdown component

ddd0f7f

Merge branch 'dropdown' of github.com:AlleeX/travix-ui-kit into dropdown

604bf2b

delete unnecessary code

ed0a273

resolve conflicts

441b6c5

edit md

7bed718

add unit tests for checkbox

fcee961

updated snapshots and added unit tests

7110208

resolved merge conflict

5136372

add unit tests

6f343c8

rollback

84efbc2

merge conflicts

685b524

AlleeX changed the title ~~Dropdown~~ Dropdown and Checkbox component Mar 24, 2017

AlleeX added 3 commits April 3, 2017 09:01

resolve merge conflict

ef5a7f2

resolve merge conflict

b8485ed

100% dropdown unit tests coverage

57a0882

asci reviewed Apr 3, 2017

View reviewed changes

components/checkbox/checkbox.js Outdated

checked,

children,

disabled,

inputAttr,

Copy link

Contributor

asci Apr 3, 2017

Choose a reason for hiding this comment

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

any use case for inputAttr and labelAttr?

AlleeX requested review from mAiNiNfEcTiOn, AlexDudar and EduardTrutsyk April 3, 2017 07:43

AlleeX self-assigned this Apr 3, 2017

AlleeX added the patch label Apr 3, 2017

remove unneeded properties from checkbox component

cbeeefd

asci reviewed Apr 3, 2017

View reviewed changes

fix PR issues

9292d7f

asci reviewed Apr 3, 2017

View reviewed changes

vendors/react-select/components.scss Outdated

@@ -0,0 +1,5 @@

@import "control";

Copy link

Contributor

asci Apr 3, 2017

Choose a reason for hiding this comment

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

is it possible to use react-select as npm module?

AlleeX added 2 commits April 3, 2017 11:50

fix unit tests

f300722

delete vendors files

a3eeb71

alinpiaa approved these changes Apr 3, 2017

View reviewed changes

AlleeX added 3 commits April 3, 2017 14:11

Merge branch 'master' into dropdown

251e176

fix building

1f7d351

fix unit test

c006d59

improve import

b38a9f6

EduardTrutsyk approved these changes Apr 3, 2017

View reviewed changes

refactoring

55a458b

mAiNiNfEcTiOn reviewed Apr 4, 2017

View reviewed changes

make code more beauty

d68bebf

spzm approved these changes Apr 4, 2017

View reviewed changes

mAiNiNfEcTiOn approved these changes Apr 4, 2017

View reviewed changes

mAiNiNfEcTiOn merged commit 8c5f72c into Travix-International:master Apr 4, 2017

mAiNiNfEcTiOn added this to Done in Components Sep 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dropdown and Checkbox component #49

Dropdown and Checkbox component #49

AlleeX commented Mar 24, 2017 •

edited

asci Apr 3, 2017

asci Apr 3, 2017

asci Apr 3, 2017

asci Apr 3, 2017

AlleeX Apr 3, 2017

asci Apr 3, 2017

AlleeX Apr 3, 2017

asci Apr 3, 2017

asci Apr 3, 2017

asci Apr 3, 2017

asci Apr 3, 2017

coveralls commented Apr 3, 2017

coveralls commented Apr 3, 2017

EduardTrutsyk Apr 3, 2017

mAiNiNfEcTiOn Apr 4, 2017

EduardTrutsyk Apr 3, 2017

AlleeX Apr 4, 2017

EduardTrutsyk Apr 3, 2017

EduardTrutsyk Apr 3, 2017

EduardTrutsyk Apr 3, 2017

EduardTrutsyk Apr 3, 2017

AlleeX Apr 4, 2017

coveralls commented Apr 4, 2017

mAiNiNfEcTiOn Apr 4, 2017

mAiNiNfEcTiOn Apr 4, 2017

mAiNiNfEcTiOn Apr 4, 2017

mAiNiNfEcTiOn Apr 4, 2017

coveralls commented Apr 4, 2017

Dropdown and Checkbox component #49

Dropdown and Checkbox component #49

Conversation

AlleeX commented Mar 24, 2017 • edited

WHAT DOES THIS PR DO:

WHERE SHOULD THE REVIEWER START:

UNIT AND/OR FUNCTIONAL TESTS:

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

coveralls commented Apr 3, 2017

coveralls commented Apr 3, 2017

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

coveralls commented Apr 4, 2017

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

coveralls commented Apr 4, 2017

AlleeX commented Mar 24, 2017 •

edited