[Chip] New component

[mbrookes]

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

New Chip component.

Features:

• Text only and "Contact" avatars.
• Contact Avatars can be image, icon (svg or font) or text.
• Optional delete icon.
• Focus shadow, click shading.
• Touch and keyboard events supported.

Still to do:

• Agree the API - currently supports both props and composability for label and Avatar. As a new component we might want to only support composability.
• Decide on required event callbacks. I took a kitchen sink approach, but we might want to narrow it down (or add others, such as drag events).
• PropTypeDescription is refusing to work, and I'm not sure why, so Ive removed it from the docs Page.js for now.
• Refactoring based on @callemall/material-ui review :sweat_smile:
• Tests. (Thanks @nathanmarks!)
• Delete button not working in Firefox. (zIndex?) (Thanks @newoga!)

Closes #1119.

[nathanmarks]

Looks pretty! I'll have a gander later :+1:

[newoga]

Looks good @mbrookes! I'll review in more detail soon!

[mbrookes]

Q: Do we need the deletable prop, or should I simply test for the presence of onRequestClose?

[oliviertassinari]

@mbrookes Is the following use case valid ?

• deletable={false}
• the Chip is focused
• the user is using the delete key, that triggers onRequestClose

[mbrookes]

@oliviertassinari - I'm not sure it is - if the chip needs to be deletable, it presumably needs to support touch devices?

[oliviertassinari]

@mbrookes In that case, I think that we can just use the onRequestClose. Do what you think is best :+1:.

[mbrookes]

Okay, I'll try that see how it looks. I'll update the prop description if that's how it works best.

[mbrookes]

@oliviertassinari - I implement that and updated the prop docs.

I also:

• Fixed the Firefox delete, and one other issues thanks to @newoga,
• Simplified the code some more
• Allow Esc key removing focus added to EnhancedButton
• Move the callback prop docs to Enhanced button, and imported from there
• Added basic tests

[mbrookes]

One more Q: Should the Chip include default left and right margin, so that it lays out nicely without additional styling? It seems like something you would need in any pretty-much any application of Chip.

[nathanmarks]

Couple quick things:

1. Why onRequestClose if the prop is deletable? onDelete seems more appropriate.
2. The delete dialog is popping up twice for me in the docs example -- this is because both onClick and onTouchTap are set on DeleteIcon probably.

Let's get this bad boy merged!

[mbrookes]

Why onRequestClose if the prop is deletable? onDelete seems more appropriate.

So the Request bit of onRequestClose differentiates from onClose in that Close hasn't happened, it has only been requested, and it's up to the event handler to take the appropriate action. an alternative might be onClickClose or onTouchTapClose, as it's more descriptive of the event that has occurred.

As for Close vs Delete - I struggled with that, and wanted to discuss it, so glad you brought it up. onXXXDelete is semantically more correct, as a Chip doesn't 'close', but onRequestClose is standard for other components, so at least it's consistent.

Happy to go with the consensus on both these point, including changing the other components to match if it comes to that.

The delete dialog is popping up twice for me in the docs example -- this is because both onClick and onTouchTap are set on DeleteIcon probably.

How the heck did that happen?! That was the other @mbrookes!

[nathanmarks]

@mbrookes tomato tomato I suppose, but I think onRequestDelete is more suitable. Consistency between components is irrelevant if the functionality represents something different. You close a drawer, you delete a chip.

[mbrookes]

How the heck did that happen?! That was the other @mbrookes!

Okay, I wasn't going mad! :smile: : https://github.com/mbrookes/material-ui/blame/chip/src/Chip/Chip.js#L231

[nathanmarks]

@mbrookes HAHAHA.

Oh man, yeah -- LOL. Oops :dancers:

[nathanmarks]

@mbrookes it sucks having to require users to use that plugin

[nathanmarks]

@mbrookes it's because I added a test to make sure onClick works, but in the tests we're obviously using simulated events and since this isn't selenium/webdriver so it doesn't trigger both. Doh.

[oliviertassinari]

@mbrookes What is missing for this PR to be merged ?

[mbrookes]

@mbrookes What is missing for this PR to be merged ?

@oliviertassinari I just need to tweak the styles slightly - that's what kicked off the whole colorManipulator.js rewrite. Something I was trying to do wasn't working as expected, and I fell down the rabbit hole when I discovered what a mess that module was.

Now that's sorted, I can finish this up.

[mbrookes]

One Q still outstanding - should I include any default right margin? Seems like every use case will need it, but not all of our components have it.

[oliviertassinari]

Now that's sorted, I can finish this up.

Awesome, I was just checking that you have all you need :).

should I include any default right margin?

@mbrookes As far as I know, none of our components have a default margin. I would say no. I think that it should be the responsibility of the container to add it.

[mbrookes]

Ok, thanks. I'll leave that as is.

[jakeboone02]

This component will frequently be part of a group of Chips, though, so if there is no default margin, I think you should at least provide some guidance about spacing between Chips on the docs. The spec appears to use 12px between Chips, on the right and on the bottom for when they wrap to multiple rows.

[mbrookes]

@jakeboone02 - 8px between chips. For simplicity for the demo I put 4px all round.

[jakeboone02]

@mbrookes From what I can tell, the spec isn't explicit about it (did I miss it?). But this example from the Single-line chips / Non-deletable chips section has 12px between them:

[mbrookes]

@jakeboone02 No, it isn't, but 8px is the defined spacing between buttons, and the spacing used in the examples if I"m not mistaken. In any case, you're free to use whatever spacing you wish.

[jakeboone02]

you're free to use whatever spacing you wish

I agree! ;)

Thanks.

[nathanmarks]

You will use the spacing that I deem appropriate. Nothing more, nothing less.

😄

[nathanmarks]

So that'd be whatever you want.

[nathanmarks]

@mbrookes squishy squashy some of those commits? -- getting hard to follow this 😄

[mbrookes]

Squashed all but the most recent 2 commits (while keeping updates to separate components in their own commit).

[nathanmarks]

@mbrookes Safari 7 came out in 2013 and received mostly security updates after that. I think we can probably take that off the "Still to do" list.

[nathanmarks]

Small thing -- do you think this should say noop instead of the empty anonymous function? https://dl.dropboxusercontent.com/s/68dx1hk2juv704d/2016-04-28%20at%207.41%20PM.png

[nathanmarks]

@mbrookes

note: This looks like a problem with our EnhancedButton because I noticed that hitting enter with the Choose an image button example is completely broken and I can't trigger other buttons more than once either, so we have an issue with keyboard accessibility for our EnhancedButton in general.

Basically -- I can't use enter to trigger a chip touchtap handler more than once without refocusing the button. (not correct behaviour, test here: https://jsfiddle.net/gakgbLb5/).

Out of scope of this PR, but something we should fix at some point.

[nathanmarks]

Are we holding off on the merge until 0.15.0 has been released? I see this is marked 0.15.1

[mbrookes]

@nathanmarks Not sure why it's milestoned for 0.15.1 specifically, but who ever says so is responsible for all future rebases. 😆

[nathanmarks]

Only bugs are out of scope of this PR, so looks good to me 👍

[jakeboone02]

In testing this, I found that props.onRequestDelete gets fired on right-click as well as left-click. I have a multi-select component built on react-virtualized-select, using Chip as the valueComponent, and that behavior is a little jarring. When I right-click I expect a context menu, not an action to be performed. The Button components behave in the expected way, for example.

I could inspect the touchtap event in my onRequestDelete handler and determine what to do from there, but I wonder if a user would ever expect a context menu-type click to remove an element from a list. If they wouldn't, then handling the left-vs-right click decision inside this component would make it easier on developers like me. :)

[mbrookes]

Thanks for the feedback Jake, I'll take a look. I'm not sure how we're handling this in other components.

[nathanmarks]

@mbrookes I didn't catch that in my review -- but I agree with @jakeboone02 that we should get a context menu.

Until we have this setup with a multi-select/autocomplete in mui, maybe being less opinionated in that aspect is the best route. For example -- if chips link to something, I can't open them in a new tab using the context menu if the right click is hijacked.

[jakeboone02]

To clarify, the problem is only with the DeleteIcon component within the Chip. Right-clicking on any other part of the Chip brings up a context menu as expected.

[mbrookes]

@jakeboone02 I've tried in Chrome, Firefox and Safari, and in all cases I get the browser context menu when I right-click on the delete Icon.

[jakeboone02]

Oh, you know what...this might be an issue with react-select and not the Chip component. I'll test some more.

[jakeboone02]

OK, looks like the issue is with react-select and not with the Chip component. Sorry for the noise!

[mbrookes]

@jakeboone02 - thanks for circling back and confirming. 👍

[jakeboone02]

Ugh...I apologize again. The problem seems to be with both components. The issue with Chip is that the onRequestDelete handler fires on any mouse/touch event. I think the reason you saw the right-click menu in your testing is that Chip doesn't do event.preventDefault().

On the new "Chips" docs page, right-clicking a DeleteIcon brings up the alert saying "You clicked the delete button," then brings up a context menu.

In my case, onRequestDelete is bound to a function that removes the chip from a multi-select value list. So the chip is removed first, and then I get a context menu.

Again, I could inspect and react to the event in my own component, but Chip could be modified to prevent this upstream. This is probably not exactly the right way to do it, but it works and shows the intent:

   handleTouchTapCloseIcon = (event) => {
+    // Only capture events from the primary mouse button or touch tap
+    if (event.nativeEvent.type === 'touchend' ||
+        (event.nativeEvent.type === 'mouseup' &&
+          event.nativeEvent.button === 0)) {
       // Stop the event from bubbling up to the `Chip`
       event.stopPropagation();
       this.props.onRequestDelete(event);
+    }
   };

[mbrookes]

@jakeboone02 When I tested, I only got the context menu, not the alert, for all three browsers that I tested, but perhaps it's OS dependant? (I'm on OS X).

Being specific about which button we're listening for seems reasonable though. (In fact we're already doing this for handleMouseDown() for the Chip itself.

That set of event filters is more specific than we have on any other component though. Not saying they're wrong, just wondering why this hasn't been a problem with other components. (Or maybe it is, and is unreported).

@nathanmarks - could use your insight here!

[nathanmarks]

@mbrookes

I can reproduce this on windows, I checked the event object too... onTouchTap is being fired from a bonafide right click event.

Looks like a bug in react-tap-event-plugin. When I change all of the examples to onClick instead of onTouchTap in ExampleSimple.js, the issue is no longer present.

Sigh

[nathanmarks]

@callemall/material-ui

We have a nasty bug in react-tap-event-plugin from what I can see.

I'm able to trigger this alert by right clicking in Windows 10 (tested in IE edge and chrome):

<button
  onTouchTap={() => alert('hello')}
>
  Hello
</button>

[mbrookes]

@nathanmarks - I don't have a windows machine to test with. If this is 'PR: needs-revision' to apply a workaround, please could you do the honours?

[nathanmarks]

@mbrookes Unfortunately there is no work-around for this -- technically this does not need a revision. The bug appears to be with react-tap-event-plugin and it is pretty severe.