[Chip] Migrate component [Avatar] Support children, change default colors #5852

mbrookes · 2016-12-29T23:01:59Z

PR has tests / docs demo, and is linted.
Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

Issues:

~~I've changes the default colours for Avatar, but directly, not with palette - I could use some guidance on what should or shouldn't be added to palette.~~
Not sure I'm applying styles to sub-components in the most optimal way - I've had to add a bunch of SubComponentClassName props. Not sure that's such a bad thing in and of itself, but again would welcome guidance.
~~I should probably separate the Avatar and Chip changes, but one was dependant on the other, so are lumped together for now.~~
~~I had some difficulty finding the right selector one of the tests: Chip.spec.js:64~~
Chip probably needs more test cases. Suggestions welcome!

~~Oh, and to agree on children vs icon prop (or both) for Avatar - I've added children so we can compare, but I'm wide open on it.~~

mbrookes · 2016-12-30T00:11:52Z

circleci is barfing on npm run test:coverage but it's fine locally...

mbrookes · 2016-12-30T13:02:52Z

Looks like whatever was up with the coverage test got fixed and the tests automatically re-run, but it was still failing on the visual regression test due to my changes to Avatar, so I was premature in removing "PR: Needs Revision". I've updated the Avatar baseline images for now.

oliviertassinari

That looks really great overall 😄 .

oliviertassinari · 2016-12-30T09:51:59Z

docs/site/src/demos/avatars/IconAvatars.js

+      <Avatar className={classes.pinkAvatar}>
+        <span className="material-icons">pageview</span>
+      </Avatar>
+


Blank line, it's not longer used to separate examples in the jsx.

oliviertassinari · 2016-12-30T21:53:19Z

src/svg-icons/navigation/cancel.js

+  </SvgIcon>
+);
+const NavigationCancel = pure(NavigationCancelIcon);
+NavigationCancel.displayName = 'NavigationCancel';


I don't think that we need that line.
Same for the other svg icons.

Any idea why we had it then? Isn't it used in React dev tools?

the pure Higher order component is already providing enough information. I think that we can save that line.

oliviertassinari · 2016-12-30T21:55:09Z

src/Chip/Chip.spec.js

+
+    it('should render a div containing an Avatar div, span and svg', () => {
+      assert.strictEqual(wrapper.is('div'), true, 'should be a div');
+      assert.strictEqual(wrapper.childAt(0).prop('component'), 'div', 'should be a div');


I much prefer the other API:

assert.strictEqual(wrapper.childAt(0).props().component, 'div', 'should be a div');

I'm curious to get your though on that.

I was struggling to find the right target for the test, and arrived at that syntax blindly, so more than happy to change to that API if this is the right thing to test for...

That looks good to me 👍 .

oliviertassinari · 2016-12-30T21:57:00Z

src/Chip/Chip.js

+  if (childCount > 1) {
+    children = React.Children.toArray(children);
+
+    if (React.isValidElement(children[0]) && children[0].type.muiName === 'Avatar') {


I see @nathanmarks coming. I know we had this discussion before.
If we had an avatar property we could save an iteration on the children and make the rendering logic simpler / faster.
Additionally, we could rely on CSS child resolution handling. A bit more fragile, but we have regressions tests.

I did catch one comment about muiName, but not the full discussion. I may be mistaken, but I don't think we save an iteration by moving from children to an avatar prop, as still you have to check it's is a valid element, and clone it to apply styling.

I may be mistaken, but I don't think we save an iteration

My bad, I have underestimated the number of iterations.
We would save a Children.count and Children.toArray call. Look at the implementation:

toArray https://github.com/facebook/react/blob/master/src/isomorphic/children/ReactChildren.js#L196

count https://github.com/facebook/react/blob/master/src/shared/utils/traverseAllChildren.js#L213

It's iterating twice.

Also, from an API point of view, I would rather depend on a named avatar property than a less obvious muiName checking when possible.

I missed this comment - I'll give it a go tomorrow.

oliviertassinari · 2016-12-30T21:58:25Z

src/Chip/Chip.js

+    label: {
+      color: palette.text,
+      // display: 'inline-flex', // For broken avatar
+      // alignItems: 'center', // For broken avatar


dead style?

oliviertassinari · 2016-12-30T22:02:17Z

src/Avatar/Avatar.js

@@ -77,6 +94,10 @@ Avatar.propTypes = {
   */
  alt: PropTypes.string,
  /**
+   * Used to render icon and text elements inside the Avatar.
+   */
+  children: PropTypes.node,


What was the issue with the icon property? I mean, we could have simply renamed it children. Why keeping both or do we need a different logic?

Not keeping both, I just didn't want to barge in and change the API without discussing it first, so it left both options in place until everyone was agreed. I'll strip it back to just children.

oliviertassinari · 2016-12-30T22:05:07Z

docs/site/src/demos/chips/Chips.js

+    margin: 6,
+  },
+  svgIcon: {
+    color: '#444',


Why not using a grey variation?

oliviertassinari · 2016-12-30T22:06:04Z

docs/site/src/demos/chips/Chips.js

+
+const styleSheet = createStyleSheet('Chips', () => ({
+  chip: {
+    margin: 6,


the smallest unity of margin / gutter is 8dp. Can we use 8 here?

I think that it would be good to have a theme property for that. We could use it on the Layout component.

oliviertassinari · 2017-01-01T12:35:23Z

src/Chip/Chip.spec.js

+
+    it('should render a div containing an Avatar div, span and svg', () => {
+      assert.strictEqual(wrapper.is('div'), true, 'should be a div');
+      assert.strictEqual(wrapper.childAt(0).prop('component'), 'div', 'should be a div');


That looks good to me 👍 .

oliviertassinari · 2017-01-01T12:36:26Z

src/Chip/Chip.js

+  const avatarClassName = classNames(classes.avatar);
+  const avatarIconClassName = classNames(classes.avatarIcon);
+  const labelClassName = classNames(classes.label, labelClassNameProp);
+  const deleteIconClassName = classNames({ [classes.deleteIcon]: onRequestDelete }, deleteIconClassNameProp);


Might worse only computing that variable only when onRequestDelete is truthy.

oliviertassinari · 2017-01-01T12:37:09Z

src/Chip/Chip.js

+  const classes = context.styleManager.render(styleSheet);
+  const className = classNames(classes.root, { [classes.clickable]: onClick }, classNameProp);
+  const avatarClassName = classNames(classes.avatar);
+  const avatarIconClassName = classNames(classes.avatarIcon);


No need to call classNames with a single argument.

oliviertassinari · 2017-01-01T12:37:21Z

src/Chip/Chip.js

+
+  const classes = context.styleManager.render(styleSheet);
+  const className = classNames(classes.root, { [classes.clickable]: onClick }, classNameProp);
+  const avatarClassName = classNames(classes.avatar);


No need to call classNames with a single argument.

I think I need to read the jss docs. :)

Is classNames just concatenating them?

classNames is an helper. That has nothing to do with jss. https://github.com/JedWatson/classnames.

oliviertassinari · 2017-01-01T12:39:08Z

src/Chip/Chip.js

+    },
+    avatarIcon: {
+      width: 19.2, // Added for jss avatar
+      height: 19.2, // Added for jss avatar


Don't we have layouting issues with non-integer?

Not sure - in master this is a dynamic style in Avatar, where the icon is 0.6 * avatar size. Since Avatar no longer supports this, we have to calculate it manually (32 * 0.6 = 19.2).

Happy to round it down if you think it could cause a problem.

oliviertassinari · 2017-01-01T12:43:23Z

src/Chip/Chip.js

+      justifyContent: 'center',
+      height: 32,
+      outline: 'none',
+      border: 0,


It's applied on a div do we need that property?

oliviertassinari · 2017-01-01T12:43:25Z

src/Chip/Chip.js

+      alignItems: 'center',
+      justifyContent: 'center',
+      height: 32,
+      outline: 'none',


It's applied on a div do we need that property?

That was carried over from master, but can't imagine why it was needed. Removed.

oliviertassinari · 2017-01-01T12:44:50Z

src/Avatar/Avatar.js

@@ -110,3 +135,5 @@ Avatar.defaultProps = {
 Avatar.contextTypes = {
  styleManager: PropTypes.object.isRequired,
 };
+
+Avatar.muiName = 'Avatar';


We could remove that line with the avatar api.

At the moment it's used in Chip to determine the child type - I'll leave it until the Chip API is solidified.

oliviertassinari · 2017-01-01T12:46:08Z

src/Avatar/Avatar.js

      borderRadius: '50%',
      overflow: 'hidden',
    },
    defaultColor: {
-      color: palette.getContrastText(palette.text.disabled),
-      background: palette.text.disabled,
+      color: 'white',


Could be using

import { white } from './colors';

Is it really worth importing 'white'? 😄

oliviertassinari · 2017-01-01T12:47:02Z

docs/site/src/demos/chips/Chips.js

+
+const styleSheet = createStyleSheet('Chips', () => ({
+  chip: {
+    margin: 6,


I think that it would be good to have a theme property for that. We could use it on the Layout component.

mbrookes · 2017-01-02T14:04:31Z

@oliviertassinari I've switched to your preferred API. Take a look!

In doing so, I also removed the iconClassName prop that I previously added to Avatar.

This makes Avatar simpler, and removes an otherwise redundant prop from Avatar (users can style the Avatar icon directly), but it does make the Chip code more complex. I'm open to your suggestion on what's preferable here.

oliviertassinari

Thanks for changing the API 👍 .
Think we are close to get it merged.

oliviertassinari · 2017-01-02T20:53:29Z

test/regressions/site/src/tests/Chip/DeletableAvatarChip.js

+    <Chip
+      avatar={<Avatar>MB</Avatar>}
+      label="SvgIcon Chip"
+      onClick={() => {}}


I don't think we need that for the regression test.

Right, no - copy pasted from the docs demo. Although that makes me curious - is it possible to regression test the hover / focus / active styles?

oliviertassinari · 2017-01-02T20:54:51Z

src/svg-icons/cancel.js

+  <SvgIcon {...props}>
+    <path
+      d="M12 2C6.47 2 2 6.47 2 12s4.47 10 10 10 10-4.47 10-10S17.53 2 12 2zm5 13.59L15.59 17 12 13.41
+      8.41 17 7 15.59 10.59 12 7 8.41 8.41 7 12 10.59 15.59 7 17 8.41 13.41 12 17 15.59z"


Is a breaking line affecting the path? I have not idea. Let's keep it that way if not.

Is a breaking line affeIs a breaking line affecting the path?

No it isn't, and in the next-docs-svg-icons PR I had manually fixed the linting errors in about half the icons which included the line-length rule, but then decided instead to add /* eslint-disable */ (I guess I could have added src/svg-icons to .eslintignore instead) and returned the icons to their generated form. For consistency I should revert this one too.

oliviertassinari · 2017-01-02T20:55:56Z

src/Chip/Chip.spec.js

+            MB
+          </Avatar>}
+          label="Text Avatar Chip"
+          onClick={() => {}}


I don't think we need that for the unit test.

oliviertassinari · 2017-01-02T21:01:09Z

src/Avatar/Avatar.js

+      color: palette.background.default,
+      backgroundColor: emphasize(palette.background.default, 0.26),
+    },
+    icon: {


What's the need for this style? It seems to be working fine with font and svg icons.
That would allow us to avoid the cloneElement.

That would allow us to avoid the cloneElement.

It would, but I've had to put it back for childrenClassName.

I agree the styles are redundant though - they're an artefact from Avatar on master where these were dynamic based on the size prop.

oliviertassinari · 2017-01-02T21:06:48Z

src/Chip/Chip.js

+    if (avatarChildren) {
+      // If Avatar has an icon, style it.
+      if (React.isValidElement(avatarChildren)) {
+        const avatarIconClassName = classNames(classes.avatarIcon, avatarChildren.props.className);


We are accessing avatarProp.props.children.props.className, dear lord, that sounds really wrong.
What about moving the responsibility by exposing a childrenClassName property to the <Avatar />? Then, we can use it here.

dear lord, that sounds really wrong.

LOL 😆 I wasn't so hot on it myself!

What about moving the responsibility by exposing a childrenClassName property to the ?

Yup, that's how I did it in a previous commit (well, iconClassName) - see my comment above:

In doing so, I also removed the iconClassName prop that I previously added to Avatar.

This makes Avatar simpler, and removes an otherwise redundant prop from Avatar (users can style the Avatar icon directly), but it does make the Chip code more complex. I'm open to your suggestion on what's preferable here.

I think making childrenClassName in Avatar hidden in the docs is probably the best compromise.

mbrookes · 2017-01-03T12:25:38Z

@oliviertassinari Okay, I've pushed those updates. My replies to your review comments may be hidden now.

A few outstanding things:

Keyboard focus isn't working and I'm not sure why. (It's working on master).
Would it make sense for a click on the delete icon to only emphasize the delete icon, and not the whole chip? If so, how would I implement that that? event.stopPropagation?
Should I disable the Avatar image regression test? A test that always fails and as a result means the rest of the tests don't run seems worse than no test!

oliviertassinari · 2017-01-03T20:09:20Z

Keyboard focus isn't working and I'm not sure why. (It's working on master).

I don't think that we need that feature at the Chip component level. For instance, look at the angular material examples: https://material.angularjs.org/latest/demo/chips.

Would it make sense for a click on the delete icon to only emphasize the delete icon

Good question, looking at the other implementation, I would answer no.
They tend to consider the chip as a single unit.

Angular:
Materializecss:

Should I disable the Avatar image regression test?

No, it's not because we broke a test that we should delete it. Fixing it would be much better.

http://0.0.0.0:3333/#/ListItem/AvatarListItem

You can reproduce that on the list demo.

I think that we are close 🚢 .

oliviertassinari · 2017-01-03T19:53:23Z

src/Avatar/Avatar.js

-    children = icon;
+  if (childrenProp) {
+    // If it's an icon, style it.
+    if (React.isValidElement(childrenProp)) {


Good, we just miss that:

if (childrenClassName && React.isValidElement(childrenProp)) {

Thanks for your patience! - It seems like my coding ability has regressed 2 years in 6 months, 😟

oliviertassinari · 2017-01-03T19:54:06Z

src/Avatar/Avatar.js

@@ -110,3 +126,4 @@ Avatar.defaultProps = {
 Avatar.contextTypes = {
  styleManager: PropTypes.object.isRequired,
 };
+


two empty lines 😬 .

oliviertassinari · 2017-01-03T19:56:47Z

src/Avatar/Avatar.spec.js

+
+    it('should render a div containing an icon', () => {
+      assert.strictEqual(wrapper.is('div'), true, 'should be a div');
+      // assert.strictEqual(wrapper.childAt(0).is('svg'), true, 'should be an svg'); // OT - help please! :)


assert.strictEqual(wrapper.find(FaceIcon).length, 1, 'should contains the icon');

oliviertassinari · 2017-01-03T19:58:56Z

src/Chip/Chip.js

+  let deleteIcon = null;
+  if (onRequestDelete) {
+    deleteIconClassName = classNames(classes.deleteIcon, deleteIconClassNameProp);
+    deleteIcon =


deleteIcon = ( <DeleteIcon className={deleteIconClassName} onClick={onRequestDelete} /> );

or

deleteIcon = <DeleteIcon className={deleteIconClassName} onClick={onRequestDelete} />;

oliviertassinari · 2017-01-03T19:59:44Z

src/Chip/Chip.js

+ *
+ * ```jsx
+ * <Chip>
+ *   <Avatar>


We need to update that example.

mbrookes · 2017-01-03T22:03:28Z

Going to pick up these loose ends tomorrow. Thanks so much for putting up with me!

oliviertassinari · 2017-01-04T20:51:46Z

Thanks so much for putting up with me!

No, thanks for taking care of that component!

mbrookes · 2017-01-04T21:06:45Z

Keyboard focus isn't working and I'm not sure why. (It's working on master).

I don't think that we need that feature at the Chip component level. For instance, look at the angular material examples: https://material.angularjs.org/latest/demo/chips.

I wouldn't read too much in to what material.angularjs.org are doing - it's a pretty ugly implementation.

Are you saying we shouldn't apply any focus style, and let the parent component watch for focus and apply focus styles to chip? That doesn't seem right to me.

Would it make sense for a click on the delete icon to only emphasize the delete icon

Good question, looking at the other implementation, I would answer no. They tend to consider the chip as a single unit.

Again, I wouldn't ready too much into that - they don't apply hover or active styes to the chip or close button, so there's no difference anyway. react-toolbox applies hover style to the close button, but nothing else.

That reminds me though - what's the preferred prop name for the close button? it's onRequestDelete at the moment, but on onCloseClick / onDeleteClick or onClickClose / onClickDeleteto match onClick are possible candidates.

Edit: NVM, we hashed that out last time, so unless there's any change of opinion, Iet's leave the name as is. HOWEVER - there's also this: #2957.

Should we channel all events through onChange? I'm not totally convinced - it would mean that the user can't just attach a handler to the appropriate callback prop, but would have to filter / decode it (for example to determine if it's a click on the chip or the delete icon).

I do still think standardizing the callback signature is a good idea though - it's a bit of a mess on master.

Should I disable the Avatar image regression test?

No, it's not because we broke a test that we should delete it. Fixing it would be much better.

I didn't say delete, I said disable. 😄

Edit: I thought this was the race condition you mentioned, but I needed to update that test for the revised Avatar API.

Edit2: Now that I've fixed the AvatarListItem regression test for the revised API, we're back to the failing ImageAvatar - which sort of proves my point: I was so used to that test failing, I didn't spot the AvatarListItem problem.

What do we do with ImageAvatar?

mbrookes · 2017-01-05T23:28:21Z

I realised the reason keyboard focus wasn't working is that Chip on master uses EnhancedButton which takes care of it, along with default keyboard event handling. I don't think ButtonBase existed when I originally started porting Chip to jss-theme-experiment.

I have started to implement the relevant functionality in Chip, but I'm wonder if I should instead use ButtonBase. It's a lot of code to drag along for a few functions, but using it is more DRY than reimplementing existing keyboard handing in Chip...

oliviertassinari · 2017-01-06T23:58:48Z

it's a pretty ugly implementation.

Well, at least, it's kind of do the job. A co-worker started implementing that component at the office with our product style guides. I will see what we can learn from that.

Are you saying we shouldn't apply any focus style, and let the parent component watch for focus

For the use case I was thinking of, chips wouldn't be keyboard focusable. The parent component would manually apply the focus state depending on it's own state.

Should we channel all events through onChange?

I'm not convinced. I don't think that some variations are going to hurt. Users will still have to look at the documentation to know how to use it correctly. Either it's the right callback property name or how to filter events.

We're back to the failing ImageAvatar

Ewww, we gonna need to address that flaky test.

I'm wonder if I should instead use ButtonBase

I think that I would definitely use the ButtonBase if I needed the ripple effect or having a button rendered in the DOM.

mbrookes · 2017-01-07T18:19:15Z

it's a pretty ugly implementation.

Well, at least, it's kind of do the job.

True - they have a Chips component. Our chip is pretty useless without one! 😅

By "ugly" though, I meant the visual appearance of the chip - I notice they have hover and click styling on their todo-list.

A co-worker started implementing that component at the office with our product style guides. I will see what we can learn from that.

👍

For the use case I was thinking of, chips wouldn't be keyboard focusable. The parent component would manually apply the focus state depending on it's own state.

I know they're not directly comparable, but thinking of Chip as a subcomponent, I'm looking at Tab and BottomNavigationButton as analogs. They're both keyboard focusable without dependance on their parent component - albeit handled by ButtonBase.

If you're happy for me to leave it as is for now, we can revisit this in the context of a ChipInput / ChipAutoComplete if anyone tackles that.

I think that I would definitely use the ButtonBase if I needed the ripple effect or having a button rendered in the DOM.

It doesn't need to ripple, or focus ripple. It should probably be a button though as it's an interactive element that can be clicked etc. That would fix the jsx-a11y/no-static-element-interactions error.

I don't think we need to use ButtonBase just for that though.

oliviertassinari

I left some minor comments. That's some great stuff 🎉 .

oliviertassinari · 2017-01-10T22:51:39Z

docs/site/src/demos/chips/ChipsArray.js

+
+    const chipData = this.state.chipData;
+    const chipToDelete = chipData.map((chip) => chip.key).indexOf(key);
+    chipData.splice(chipToDelete, 1);


I'm not sure it's a great example. splice mutate the array. It would be good to clone chipData first 😁 .

const chipData = [...this.state.chipData];

Line 47? (Or am I misunderstanding?)

Yes, indeed

Right, right. I'm surprised React doesn't scream about that!

oliviertassinari · 2017-01-10T22:54:07Z

docs/site/src/demos/chips/Chips.js

+  const classes = context.styleManager.render(styleSheet);
+  return (
+    <div className={classes.row}>
+      <Chip


This element is focusable but don't have :focus state.
Is that expected?

oliviertassinari · 2017-01-10T22:55:21Z

docs/api/Chip/Chip.md

+| onClick | function |  | Callback function fired when the `Chip` element is clicked.<br><br>**Signature:**<br>`function(event: object) => void`<br>*event:* TouchTap event targeting the element. |
+| onRequestDelete | function |  | Callback function fired when the delete icon is clicked. If set, the delete icon will be shown.<br><br>**Signature:**<br>`function(event: object) => void`<br>*event:* `touchTap` event targeting the element. |
+
+Other properties (not documented) are applied to the root element.


We might need to rebase on next as I think we have improved that wording thanks to you 👍

oliviertassinari · 2017-01-10T22:56:40Z

docs/site/src/demos/lists/FolderList.js

          <ListItemText primary="Photos" secondary="Jan 9, 2016" />
        </ListItem>
        <ListItem button>
-          <Avatar icon={<span className="material-icons">folder</span>} />
+          <Avatar ><span className="material-icons">folder</span></Avatar>


<Avatar><span className="material-icons">folder</span></Avatar>

oliviertassinari · 2017-01-10T22:59:58Z

src/Chip/Chip.js

+
+  let chipRef;
+
+  const handleDeleteIconClick = (event) => {


I have never seen that pattern before. Interesting.
We could also share the function definition between renders by accepting a props argument.

We could also share the function definition between renders by accepting a props argument.

Sorry, could you explain please?

If we change the signature to the following, I think that we would be able to allocate the function once.

const handleDeleteIconClick = (event, props) => {

Sorry you're going to have to spell it out. 😊

Why pass props when we're not using it, and how does that affect allocation?

Right, my bad, that's not going to change anything as used like this onClick={handleDeleteIconClick} />;.
Using a class, on the other hand, could help.

Using a class, on the other hand, could help.

For Chip?

It's a trade-off I guess. I wonder how much difference it would make, as instead we're dragging lifecycle methods along for the ride.

oliviertassinari · 2017-01-10T23:00:57Z

src/Chip/Chip.js

+  let deleteIcon = null;
+  if (onRequestDelete) {
+    deleteIconClassName = classNames(classes.deleteIcon, deleteIconClassNameProp);
+    deleteIcon = (<DeleteIcon className={deleteIconClassName} onClick={handleDeleteIconClick} />);


I don't think we need the parentheses ().

oliviertassinari · 2017-01-10T23:02:15Z

src/Chip/Chip.js

+    <button
+      className={className}
+      onClick={onClick}
+      tabIndex={0}


I'm wondering if it shouldn't be -1 when the element don't have onClick or onRequestClose callbacks.

Yes, that should fix the focus issue you mentioned.

It fixed the issue, but now I'm wondering if there should no tabIndex at all when the element don't have onClick or onRequestClose callbacks?

I'm wondering, isn't tabIndex={0} doing nothing on an enable button. In that case yes, it could be undefined.

oliviertassinari · 2017-01-11T07:51:02Z

docs/site/src/demos/chips/ChipsArray.js

+
+    const chipData = this.state.chipData;
+    const chipToDelete = chipData.map((chip) => chip.key).indexOf(key);
+    chipData.splice(chipToDelete, 1);


Yes, indeed

oliviertassinari · 2017-01-11T07:52:44Z

docs/site/src/demos/chips/ChipsArray.js

+    }
+
+    const chipData = this.state.chipData;
+    const chipToDelete = chipData.map((chip) => chip.key).indexOf(key);


I think that it would be better like this, only reading memory and not allocating anything.

const chipToDelete = chipData.indexOf(chipData.find((chip) => chip.key === key)));

oliviertassinari · 2017-01-11T20:06:37Z

docs/site/src/demos/lists/FolderList.js

          <ListItemText primary="Photos" secondary="Jan 9, 2016" />
        </ListItem>
        <ListItem button>
-          <Avatar icon={<FolderIcon />} />
+          <Avatar> <FolderIcon /></Avatar>


Merge conflict foobar. Fixed.

oliviertassinari · 2017-01-11T20:11:09Z

docs/site/src/demos/chips/ChipsArray.js

+      return;
+    }
+
+    const chipData = Object.assign([], this.state.chipData);


That's probably the only the only reference to Object.assign in the code base. I would rather use the destructuration.

Oh, interesting. That sounds like a much better idea.

(I did wonder where all the Object.assigns were in next given how heavily we used it on master 😄 )

[Avatar] Support children, change default colors

muibot added the PR: Needs Review label Dec 29, 2016

mbrookes added the component: chip This is the name of the generic UI component, not the React module! label Dec 29, 2016

mbrookes changed the title ~~[Chip] Migrate Chip [Avatar] Support children, change default colors~~ [Chip] Migrate component [Avatar] Support children, change default colors Dec 29, 2016

muibot added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI PR: Needs Review and removed PR: Needs Review labels Dec 29, 2016

mbrookes added next and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Dec 30, 2016

muibot added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI PR: Needs Review and removed PR: Needs Review labels Dec 30, 2016

mbrookes removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Dec 30, 2016

muibot added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI PR: Needs Review and removed PR: Needs Review labels Dec 30, 2016

mbrookes removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Dec 30, 2016

muibot added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Dec 30, 2016

oliviertassinari reviewed Dec 30, 2016

View reviewed changes

muibot added the PR: Needs Review label Dec 31, 2016

oliviertassinari requested changes Jan 1, 2017

View reviewed changes

oliviertassinari removed the PR: Needs Review label Jan 1, 2017

muibot added the PR: Needs Review label Jan 1, 2017

mbrookes removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Jan 2, 2017

muibot added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Jan 2, 2017

oliviertassinari requested changes Jan 2, 2017

View reviewed changes

oliviertassinari reviewed Jan 3, 2017

View reviewed changes

mbrookes removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Jan 9, 2017

oliviertassinari reviewed Jan 10, 2017

View reviewed changes

oliviertassinari reviewed Jan 11, 2017

View reviewed changes

muibot added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI PR: Needs Review and removed PR: Needs Review labels Jan 11, 2017

mbrookes removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Jan 11, 2017

oliviertassinari reviewed Jan 11, 2017

View reviewed changes

muibot added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI PR: Needs Review and removed PR: Needs Review labels Jan 11, 2017

oliviertassinari approved these changes Jan 11, 2017

View reviewed changes

oliviertassinari added PR: Review Accepted and removed PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Jan 11, 2017

[Chip] Migrate component

943f95f

[Avatar] Support children, change default colors

muibot added the PR: Needs Review label Jan 11, 2017

mbrookes merged commit bdc85ff into mui:next Jan 11, 2017

mbrookes deleted the next-chip branch January 11, 2017 23:29

mbrookes removed the PR: Needs Review label Jan 11, 2017

mbrookes mentioned this pull request Jan 15, 2017

[Chip] Port component #4786

Closed

4 tasks

[Chip] Migrate component [Avatar] Support children, change default colors #5852

[Chip] Migrate component [Avatar] Support children, change default colors #5852

Conversation

mbrookes commented Dec 29, 2016 • edited Loading

mbrookes commented Dec 30, 2016 • edited Loading

mbrookes commented Dec 30, 2016 • edited Loading

oliviertassinari 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

mbrookes Dec 31, 2016 • 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

oliviertassinari Jan 1, 2017 • 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

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

mbrookes commented Jan 2, 2017

oliviertassinari 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

mbrookes Jan 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrookes Jan 3, 2017 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Jan 2, 2017 • edited Loading

Choose a reason for hiding this comment

mbrookes Jan 3, 2017 • edited Loading

Choose a reason for hiding this comment

mbrookes commented Jan 3, 2017

oliviertassinari commented Jan 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

mbrookes Jan 4, 2017 • 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

mbrookes commented Jan 3, 2017

oliviertassinari commented Jan 4, 2017

mbrookes commented Jan 4, 2017 • edited Loading

mbrookes commented Jan 5, 2017

oliviertassinari commented Jan 6, 2017 • edited Loading

mbrookes commented Jan 7, 2017 • edited Loading

oliviertassinari left a comment

mbrookes commented Dec 29, 2016 •

edited

Loading

mbrookes commented Dec 30, 2016 •

edited

Loading

mbrookes commented Dec 30, 2016 •

edited

Loading

mbrookes Dec 31, 2016 •

edited

Loading

oliviertassinari Jan 1, 2017 •

edited

Loading

mbrookes Jan 3, 2017 •

edited

Loading

mbrookes Jan 3, 2017 •

edited

Loading

oliviertassinari Jan 2, 2017 •

edited

Loading

mbrookes Jan 3, 2017 •

edited

Loading

mbrookes Jan 4, 2017 •

edited

Loading

mbrookes commented Jan 4, 2017 •

edited

Loading

oliviertassinari commented Jan 6, 2017 •

edited

Loading

mbrookes commented Jan 7, 2017 •

edited

Loading