Add support for other Material UI variants (outlined, filled) #236
Conversation
|
Hmm, I'm also getting the same test failure when I run the tests on Master. Anyone have an idea of what's going on? Edit: Fixed the tests. I needed to upgrade the enzyme package, per this issue: enzymejs/enzyme#1411 |
There was a problem hiding this comment.
Great work, @neilpoulin 🎉 This is a really good first contribution to this project.
It's a bit hard to review the changes due to a bunch of unrelated formatting changes, though. Why are they needed? Some might even trigger our linter. Could you revert them? :)
src/ChipInput.js
Outdated
| @@ -161,9 +206,9 @@ class ChipInput extends React.Component { | |||
| if (this.state.focusedChip != null) { | |||
| this.setState({ focusedChip: null }) | |||
| } | |||
| } | |||
| }; | |||
There was a problem hiding this comment.
I'm pretty sure that this won't lint. We use standard linter (npm run lint)
There was a problem hiding this comment.
Apologies for the added semis and other formatting - my IDE was not playing nicely with your formatting standards, and I wasn't being careful enough to notice these changes. I'll get them cleaned out!
There was a problem hiding this comment.
Don't worry. I really like this PR, no matter the formatting. 😉 npm run lint -- --fix can probably fix most of the issues.
|
@neilpoulin Could you update the TS definitions? I forgot about them. |
|
sure thing, i'll get another PR up for that shortly. Thanks for the responsiveness! Any idea when this might be available on NPM? |
|
Hi @leMaik & @neilpoulin ! The build crashes with this feature. Normal ? Thanks. |
|
@julienexcusemyparty That's obviously not normal. Are you sure that the crash you observe is due to this PR? Could you please provide an error log? 🔮 Edit: Doesn't crash for me. Please open a new issue and provide a log. |
Support outlined and filled variants. Fixes #234.
Filled:
Outlined:
