feat(chips): Add new component #117

bonniezhou · 2018-06-25T23:36:30Z

Add basic working component for Chips, including Chip and ChipSet. Does not include any variants.

moog16

Can you add tests for adapter methods for both chip and chipset por favor

moog16 · 2018-06-25T23:48:29Z

packages/chips/chip-set/index.js

+      classList: new Set(),
+      chips: this.props.labels.map((label) => {
+        return {
+          label: label,


change this to just

{ label, ... }

moog16 · 2018-06-25T23:49:02Z

packages/chips/chip-set/index.js

+    this.maxId = 0;
+    this.state = {
+      classList: new Set(),
+      chips: this.props.labels.map((label) => {


Since you're passed props, just reference that:

chips: props.labels.map...

moog16 · 2018-06-25T23:50:11Z

packages/chips/chip-set/index.js

+  render() {
+    return (
+      <div className={this.classes}>
+        {this.state.chips.map(this.renderInputChip.bind(this))}


Do you need to bind this. You don't use it in renderInputChip. I actually think render methods always have a reference to this.

Oops, copied this over from an experimental branch. Later when we add input chips this will be used in the render method so we can bind it then.

moog16 · 2018-06-25T23:52:17Z

packages/chips/chip-set/index.js

+  }
+
+  get classes() {
+    const {classList} = this.state;


you actually don't need classList. You don't have an add/remove.

moog16 · 2018-06-25T23:52:49Z

packages/chips/chip/index.js

+
+  constructor(props) {
+    super(props);
+    this.state = {


move this as a class property (not in the constructor)

moog16 · 2018-06-25T23:54:17Z

packages/chips/package.json

@@ -0,0 +1,29 @@
+{
+  "name": "@material/react-chips",
+  "version": "0.2.0",


Since this is new I think lerna wants it to be 0.0.0. When we publish again it'll auto generate the version.

moog16 · 2018-06-25T23:55:31Z

packages/chips/package.json

+    "url": "https://github.com/material-components/material-components-web-react.git"
+  },
+  "dependencies": {
+    "@material/chips": "^0.35.0",


I need to update the docs when adding a new component (actually need a doc), but please add chips to the webpack config for building: https://github.com/material-components/material-components-web-react/blob/master/packages/webpack.config.js#L72

moog16 · 2018-06-25T23:57:20Z

test/unit/chips/index.test.js

+
+test('renders chip set and chip', () => {
+  const wrapper = shallow(<ChipSet labels={['Hello']} />);
+  assert.isOk(wrapper.find('.mdc-chip-set'));


I think there is an assert.exists()

codecov-io · 2018-06-27T17:38:13Z

Codecov Report

Merging #117 into master will decrease coverage by 1.28%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##           master     #117      +/-   ##
==========================================
- Coverage   99.05%   97.77%   -1.29%     
==========================================
  Files          18       20       +2     
  Lines         637      673      +36     
  Branches       53       53              
==========================================
+ Hits          631      658      +27     
- Misses          6       15       +9

Impacted Files	Coverage Δ
packages/chips/chip/index.js	`65% <65%> (ø)`
packages/chips/chip-set/index.js	`87.5% <87.5%> (ø)`

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 a53fc7d...15071f3. Read the comment docs.

moog16

Please add some adapter method tests. Gotta keep up the coverage :)

moog16 · 2018-06-27T19:32:46Z

package.json

@@ -45,6 +45,7 @@
    "@google-cloud/storage": "^1.6.0",
    "@material/button": "^0.36.0",
    "@material/card": "^0.36.0",
+    "@material/chips": "^0.36.0",


does chips have a version 0.36.0? It is 0.35.x in package-lock

amsheehan

I had a few cycles and thought I'd take a crack at the ol' open source. This looks so sweet. :)

Just a few nit picky details that caught my eye. Super jeal of how famous y'all are now with Dan Abramov

amsheehan · 2018-06-27T19:57:08Z

packages/chips/README.md

+## Installation
+
+```
+npm install @material/react-chips


Do we want to include --save? For completeness?

No, --save has not existed for several major versions of npm.

amsheehan · 2018-06-27T20:00:48Z

packages/chips/README.md

+Prop Name | Type | Description
+--- | --- | ---
+className | String | Classes to be applied to the chip set element.
+labels | Array | Array of text to be displayed in each chip.


Language in the description is a little ambiguous. Maybe "An array of strings. Each string has a corresponding chip whose label will be set to the value of that string."

amsheehan · 2018-06-27T20:03:33Z

packages/chips/chip-set/index.js

+
+import Chip from '../chip';
+
+export default class ChipSet extends Component {


In the README's example, you use a different import style for Component, e.g.:

import React from 'react'; class Foo extends React.Component {...}

Suggest changing the example in the README to use this code style instead.

amsheehan · 2018-06-27T20:08:58Z

packages/chips/chip-set/index.js

+    super(props);
+    this.maxId = 0;
+    this.state = {
+      chips: props.labels.map((label) => {


It is an anti-pattern to set props that will never change to state. Consider using props directly in the render method.

amsheehan · 2018-06-27T20:09:49Z

packages/chips/chip-set/index.js

+
+  constructor(props) {
+    super(props);
+    this.maxId = 0;


What is this used for?

amsheehan · 2018-06-27T20:19:41Z

packages/chips/chip-set/index.js

+};
+
+ChipSet.defaultProps = {
+  className: '',


Is this necessary?

amsheehan · 2018-06-27T20:21:53Z

packages/chips/chip/index.js

+import {MDCChipFoundation} from '@material/chips';
+
+export class Chip extends Component {
+


Did eslint complain about starting this with an empty line?

amsheehan · 2018-06-27T20:23:13Z

packages/chips/chip/index.js

+  children: PropTypes.string,
+};
+
+Chip.defaultProps = {


I'm not sure you'll need all of these. Possibly initRipple, because I don't know the internals of it.

amsheehan · 2018-06-27T20:24:39Z

packages/chips/package.json

+  "dependencies": {
+    "@material/chips": "^0.36.0",
+    "@material/react-ripple": "^0.2.0",
+    "classnames": "^2.2.5",


Do we have a way of making sure that minor version changes to these will not cause problems for the components?

amsheehan · 2018-06-27T20:26:12Z

test/unit/chips/Chip.test.js

+
+test('#adapter.addClass adds class to state.classList', () => {
+  const wrapper = mount(<Chip>Jane Doe</Chip>);
+  wrapper.instance().foundation_.adapter_.addClass('test-class-name');


Should we be accessing private properties in a test? Is there a way to spy on addClass? I'm seeing this pattern here, in the next test, and one of the ChipSet tests.

Changed the ChipSet test to use its public adapter getter, but I'm not sure if there's a way to do the same for Chip since it's an HOC wrapped in withRipple.

Are you using Jest to run the tests? If so, there are some handy automatic mocking functions you can use and pass arbitrary props down to Chip and check its className property.

http://jestjs.io/docs/en/es6-class-mocks#automatic-mock

We're not using Jest :( we're using Karma like in the MDC Web repo

moog16 · 2018-06-29T21:29:28Z

packages/chips/chip-set/index.js

+  render() {
+    return (
+      <div className={this.classes}>
+        {this.props.labels.map((label, index) => this.renderChip(label, index))}


this can reduce to: this.props.labels.map(this.renderChip)

moog16

Looks good pending screenshot tests and single comment.

bonniezhou added 17 commits June 18, 2018 10:53

Set up boilerplate for new component

57d165d

one chip no ripple

d9ea92c

Get chip set working

8351470

Remove choice and filter

9f26358

Remove selected

698a92d

Rename class and set proptypes

46980af

resolve classnames

a6e7fb5

omg it works

7c499f9

add custom colors

05569da

Don't pass entire chip object to chip

14bc5ef

update readme

83388ed

Tests WIP

f487e4a

Change labels to children

fa2a8f1

WIP tests

3e88629

tests

8ed862f

Remove access of private handlers

58eab6d

Update README

7356cb9

bonniezhou requested a review from moog16 June 25, 2018 23:36

Merge branch 'master' into feat/chips-simple

31fc38c

moog16 suggested changes Jun 26, 2018

View reviewed changes

bonniezhou added 2 commits June 25, 2018 17:11

Lint

ca569fa

all the fixes

28846ce

fix test

b8dcae9

moog16 suggested changes Jun 27, 2018

View reviewed changes

bonniezhou added 2 commits June 27, 2018 13:22

Add adapter tests

e31944c

Update package lock

a5955f0

amsheehan reviewed Jun 27, 2018

View reviewed changes

bonniezhou added 2 commits June 27, 2018 15:46

Remove state from chip set and remove defaultProps

01c086e

README updates

61ee555

fix tests

5012dd8

moog16 mentioned this pull request Jun 29, 2018

Update all component readmes to include --save #120

Closed

moog16 reviewed Jun 29, 2018

View reviewed changes

Update index.scss

62da397

moog16 approved these changes Jun 29, 2018

View reviewed changes

bonniezhou added 3 commits July 2, 2018 10:48

Simplify nit

208dfcb

merge with master

0bd30a1

Add goldens for chips

15071f3

bonniezhou merged commit 410da30 into master Jul 2, 2018

moog16 deleted the feat/chips-simple branch July 3, 2018 16:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(chips): Add new component #117

feat(chips): Add new component #117

bonniezhou commented Jun 25, 2018

moog16 left a comment

moog16 Jun 25, 2018

moog16 Jun 25, 2018

moog16 Jun 25, 2018

bonniezhou Jun 27, 2018

moog16 Jun 25, 2018

moog16 Jun 25, 2018

moog16 Jun 25, 2018

moog16 Jun 25, 2018

moog16 Jun 25, 2018

codecov-io commented Jun 27, 2018 •

edited

Loading

moog16 left a comment

moog16 Jun 27, 2018

amsheehan left a comment

amsheehan Jun 27, 2018

kfranqueiro Jul 2, 2018

amsheehan Jun 27, 2018

amsheehan Jun 27, 2018

amsheehan Jun 27, 2018

amsheehan Jun 27, 2018

amsheehan Jun 27, 2018

amsheehan Jun 27, 2018

amsheehan Jun 27, 2018

amsheehan Jun 27, 2018

amsheehan Jun 27, 2018

bonniezhou Jun 28, 2018

amsheehan Jun 28, 2018

bonniezhou Jun 29, 2018

moog16 Jun 29, 2018

moog16 left a comment


		import Chip from '../chip';

		export default class ChipSet extends Component {

		import {MDCChipFoundation} from '@material/chips';

		export class Chip extends Component {

feat(chips): Add new component #117

feat(chips): Add new component #117

Conversation

bonniezhou commented Jun 25, 2018

moog16 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jun 27, 2018 • edited Loading

Codecov Report

moog16 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amsheehan 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

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

moog16 left a comment

Choose a reason for hiding this comment

codecov-io commented Jun 27, 2018 •

edited

Loading