feat(select): add component #169

moog16 · 2018-07-23T23:29:52Z

fixes #128

codecov-io · 2018-07-23T23:37:35Z

Codecov Report

Merging #169 into master will decrease coverage by 0.09%.
The diff coverage is 98.07%.

@@            Coverage Diff            @@
##           master     #169     +/-   ##
=========================================
- Coverage   98.79%   98.69%   -0.1%     
=========================================
  Files          18       20      +2     
  Lines         664      768    +104     
  Branches       62       69      +7     
=========================================
+ Hits          656      758    +102     
- Misses          8       10      +2

Impacted Files	Coverage Δ
packages/select/index.js	`100% <100%> (ø)`
packages/select/NativeControl.js	`92.3% <92.3%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9dde169...07f647e. Read the comment docs.

moog16 · 2018-07-23T23:38:01Z

test/screenshot/webpack-bundles.js

@@ -21,6 +21,14 @@ module.exports.bundle = function(testPath, outputPath) {
            {
              loader: 'css-loader',
            },
+            {


screenshot tests looked strange without this...I noticed that the appearance: none rule was not being applied. Chrome needs the -webkit-appearance: none rule to apply.

moog16 · 2018-07-24T00:50:37Z

~~this might also be an issue: #165~~

EDIT: I don't think this is an issue, since select only has a set amount of values it can be. The way in which the value is managed is slightly different than text-field, which avoids the issue.

lynnmercier · 2018-07-24T17:09:34Z

packages/select/NativeControl.js

+
+  handleFocus = (e) => {
+    const {foundation, onFocus} = this.props;
+    foundation.focusHandler_(e);


I think you need to open a PR in MDCW to change the MDCSelectFoundation to have handleFocus and handleBlur methods.

This "private" method focusHandler_ is not something we can depend on, because it is not documented in the MDCW documentation.

lynnmercier · 2018-07-24T17:10:10Z

packages/select/NativeControl.js

+    const {foundation, handleValueChange, onChange} = this.props;
+    const {value} = e.target;
+
+    foundation.selectionHandler_(e);


The MDCW PR should also change selectionHandler_ to handleChange

lynnmercier · 2018-07-24T17:10:42Z

packages/select/NativeControl.js

+  onBlur: PropTypes.func,
+  onChange: PropTypes.func,
+  onFocus: PropTypes.func,
+  setDisabled: PropTypes.func,


should this be onDisabled? instead of setDisabled

The convention is that on indicates an event. Disabled is just a state or attribute, so it seems more appropriate to call it setDisabled.

lynnmercier · 2018-07-24T17:11:05Z

packages/select/NativeControl.js

+  className: PropTypes.string,
+  children: PropTypes.node,
+  disabled: PropTypes.bool,
+  handleValueChange: PropTypes.func,


Whats the difference between handleValueChange and onChange??

onChange is the event handler - its a React term so we shouldn't change that one. handleValueChange could be renamed. Does renaming to syncSelectValue make more sense? It's to update the parent's value.

lynnmercier · 2018-07-24T17:12:23Z

packages/select/NativeControl.js

+  children: PropTypes.node,
+  disabled: PropTypes.bool,
+  handleValueChange: PropTypes.func,
+  foundation: PropTypes.shape({


I assume the foundation is a prop because you need the parent component to pass the foundation to this NativeControl?

Yes, we call 2 functions from the foundation.

lynnmercier · 2018-07-24T17:17:54Z

packages/select/index.js

+      hasClass: (className) => this.classes.split(' ').includes(className),
+      isRtl: this.getIsRtl,
+      getValue: (value) => this.state.value,
+      // setValue, getSelectedIndex, setSelectedIndex aren't needed


should we file an issue on MDCW to remove getSelectedIndex and setSelectedIndex from the adapter? Maybe they should only be in the Vanilla component.

lynnmercier · 2018-07-24T17:21:28Z

packages/select/index.js

+      // and selectedindex themselves
+    };
+
+    const labelAdapter = {


you didn't implement shakeLabel...is that on purpose?

lynnmercier · 2018-07-24T17:21:57Z

packages/select/index.js

+
+    const lineRippleAdapter = {
+      activateBottomLine: () => this.setState({activeLineRipple: true}),
+      deactivateBottomLine: () => this.setState({activeLineRipple: false}),


didn't implement setLineRippleTransformOrigin... is that on purpose?

MDC Select doesn't call that specific line ripple adapter method. It was filed as a bug a week-ish ago.

lynnmercier · 2018-07-24T17:22:52Z

packages/select/index.js

+    );
+  }
+
+  getIsRtl = () => {


this is copy/pasted from text field. You should move this to a RTL package, and then reference it from both text field and select

You mean create a component and wrap text-field and select? The only thing in that component would be this isRtl method. Is that what you are thinking?

lynnmercier · 2018-07-24T17:27:26Z

packages/select/index.js

+    });
+  }
+
+  renderWithLabel(selectContainer) {


just renderLabel

moog16 · 2018-08-01T06:24:37Z

TODO: Update main readme table of info

bonniezhou · 2018-08-01T18:40:39Z

packages/select/index.js

+      // floating label state
+      labelIsFloated: false,
+      labelWidth: 0,
+      setLineRippleTransformOrigin: (lineRippleCenter) => this.setState({lineRippleCenter}),


Why is this in state?

bonniezhou · 2018-08-01T18:45:28Z

packages/select/index.js

+      },
+      hasClass: (className) => this.classes.split(' ').includes(className),
+      isRtl: () => this.props.isRtl,
+      getValue: (value) => this.state.value,


getValue should take no params

bonniezhou · 2018-08-01T18:51:18Z

packages/select/index.js

+    });
+  }
+
+  renderLabel(selectContainer) {


unused param

bonniezhou · 2018-08-01T18:52:18Z

packages/select/index.js

+export default class Select extends React.Component {
+
+  foundation_ = null;
+  selectContainerElement_ = React.createRef();


Can we remove this ref? It doesn't look like it's being used

bonniezhou · 2018-08-01T19:00:35Z

packages/select/NativeControl.js

+    handleFocus: PropTypes.func,
+    handleBlur: PropTypes.func,
+  }),
+  id: PropTypes.string,


When is id being used?

Its used for floatingLabel and associating it for screenreaders

The id should also be passed to the NativeControl from Select then (right now it's only passed to FloatingLabel)

It is being passed to the id through {...otherProps}

…-components-web-react into feat/select

bonniezhou · 2018-08-01T21:37:57Z

packages/select/NativeControl.js

+    handleFocus: PropTypes.func,
+    handleBlur: PropTypes.func,
+  }),
+  id: PropTypes.string,


The id should also be passed to the NativeControl from Select then (right now it's only passed to FloatingLabel)

bonniezhou · 2018-08-02T19:15:24Z

packages/select/README.md

+floatingLabelClassName | String | An optional class added to the floating label element.
+id | String | Id of the `<select>` element.
+label | String | Mandatory. Label text that appears as the floating label.
+isRtl | Boolean | If toggle from false to true or vice-versa, it will recalculate the notched outline element to the appropriate width and positioning.


Description should be "Whether the direction of the select is set to RTL."

bonniezhou · 2018-08-02T19:16:30Z

packages/select/README.md

+Sass mixins may be available to customize various aspects of the Components. Please refer to the
+MDC Web repository for more information on what mixins are available, and how to use them.
+
+[Advanced Sass Mixins]([Advanced Sass Mixins](https://github.com/material-components/material-components-web/blob/master/packages/mdc-select/README.md#sass-mixins))


There's an extra [Advanced Sass Mixins] here

bonniezhou · 2018-08-02T19:27:29Z

packages/select/README.md

+  }
+}
+```
+


We should add a note somewhere about the first option being a placeholder like here in MDC Web.

bonniezhou · 2018-08-02T19:45:14Z

test/unit/select/NativeInput.test.js

+import {shallow} from 'enzyme';
+import NativeControl from '../../../packages/select/NativeControl';
+
+suite('Select Native Input');


rename this test suite and file to be NativeControl.test.js

bonniezhou · 2018-08-02T19:46:34Z

test/unit/select/NativeInput.test.js

+  td.verify(syncSelectValue(value), {times: 1});
+});
+
+test('calls props.setDisabled props.disabed', () => {


"calls props.setDisabled if props.disabled is true"

lynnmercier · 2018-08-02T21:33:05Z

packages/select/NativeControl.js

+      children,
+      foundation,
+      value,
+      syncSelectValue,


remove syncSelectedValue....the parent component should use the onChange prop callback for this

lynnmercier · 2018-08-02T21:34:31Z

packages/select/NativeControl.js

+      foundation,
+      value,
+      syncSelectValue,
+      setDisabled,


change this to handleDisabled, its a callback to an event essentially

lynnmercier · 2018-08-02T21:38:07Z

packages/select/README.md

+
+> NOTE: In order to get access to the value, you must add an `onChange` handler, which accepts an event and updates the value of the select as shown above.
+
+#### Shorthand options


Can you add some more information in the README about the use case where JSON comes from the API

…l-components-web-react into feat/select

lynnmercier · 2018-08-03T17:20:40Z

packages/select/NativeControl.js

@@ -5,17 +5,16 @@ import classnames from 'classnames';
 export default class NativeControl extends React.Component {

  componentDidMount() {


try removing this, i dont think we need it

Matt Goo added 16 commits July 19, 2018 11:33

WIP: select

ca0d280

WIP: select

9539089

Merge branch 'master' into feat/select

9015e00

WIP: added screenshot tests

e175e62

WIP: screenshot

aeb1bce

WIP: lint

ff02bea

WIP: screenshot

385ab54

WIP: tests

9051e42

Merge branch 'master' into feat/select

127cc96

WIP: remove wrapper comoponentt

92662d8

WIP: more test

964f2b5

WIP: add test

d65ea16

WIP: test

f8d5841

WIP: tests

1ed8264

Merge branch 'master' into feat/select

1e30358

WIP: update select readme

c4f2f7d

moog16 commented Jul 23, 2018

View reviewed changes

Matt Goo added 2 commits July 23, 2018 16:38

WIP: update some JS

67b2b7d

Update README.md

3fc263d

lynnmercier suggested changes Jul 24, 2018

View reviewed changes

Matt Goo added 8 commits July 27, 2018 13:20

WIP: changes to select with new updates in next release

71f376a

Merge branch 'master' into feat/select

eb7bcc2

WIP: add bidirection package

9060082

WIP: remove bidirection

00714cd

WIP: removed bidirection

0a44103

WIP: made isRtl a prop

25b49b7

WIP: fix typos

0d79d9c

WIP: add line ripple set origin method

bb5936e

Merge branch 'master' into feat/select

4960e9b

bonniezhou suggested changes Aug 1, 2018

View reviewed changes

Matt Goo added 4 commits August 1, 2018 13:21

Merge branch 'master' into feat/select

d200ce6

Merge branch 'master' into feat/select

690b54b

WIP: pr changes

49116c4

Merge branch 'feat/select' of github.com:material-components/material…

07efcbd

…-components-web-react into feat/select

bonniezhou self-assigned this Aug 1, 2018

bonniezhou reviewed Aug 2, 2018

View reviewed changes

lynnmercier suggested changes Aug 2, 2018

View reviewed changes

lynnmercier and others added 5 commits August 2, 2018 17:40

Merge branch 'master' into feat/select

5cf858e

WIP: tests

72213ad

tMerge branch 'feat/select' of github.com:material-components/materia…

41e80ce

…l-components-web-react into feat/select

WIP: screenshot test

6fc484b

Merge branch 'master' into feat/select

7e8520f

lynnmercier approved these changes Aug 3, 2018

View reviewed changes

bonniezhou approved these changes Aug 3, 2018

View reviewed changes

Matt Goo added 2 commits August 3, 2018 13:43

Merge branch 'master' into feat/select

27ad69a

WIP: remove handleDisabled call from nativeControl componentDidMount

07f647e

moog16 merged commit e619e52 into master Aug 3, 2018

moog16 deleted the feat/select branch August 3, 2018 21:07


		> NOTE: In order to get access to the value, you must add an `onChange` handler, which accepts an event and updates the value of the select as shown above.

		#### Shorthand options

		@@ -5,17 +5,16 @@ import classnames from 'classnames';
		export default class NativeControl extends React.Component {

		componentDidMount() {

feat(select): add component #169

feat(select): add component #169

Conversation

moog16 commented Jul 23, 2018

codecov-io commented Jul 23, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

moog16 commented Jul 24, 2018 • 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

moog16 Jul 30, 2018 • 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

moog16 commented Aug 1, 2018

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

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

codecov-io commented Jul 23, 2018 •

edited

Loading

moog16 commented Jul 24, 2018 •

edited

Loading

moog16 Jul 30, 2018 •

edited

Loading