fix(chips): id shuold be required prop #649
Conversation
Codecov Report
@@ Coverage Diff @@
## rc0.10.0 #649 +/- ##
===========================================
+ Coverage 94.8% 94.8% +<.01%
===========================================
Files 68 68
Lines 2849 2851 +2
Branches 429 430 +1
===========================================
+ Hits 2701 2703 +2
Misses 51 51
Partials 97 97
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
thanks for fixing...yes this is definitely required in the choice, filter, and input, but not basic. I think we should keep this optional for the basic variant as this will also be a breaking change for some people.
What if we instead throw an error in the componentDidMount()?
if ((choice || filter || input) && !id) {
throw new Error('should have id') // better error message.
}
Please also update the example <Chip /> in https://github.com/material-components/material-components-web-react/tree/master/packages/chips#input-chips
|
@moog16 The problem with throwing an error on componentDidMount is that the chips themselves don't know what sort of variant they are. This information is passed in the If that sounds correct to you I will go ahead and make that change. edit: Perhaps another way would be for ChipSet to pass type prop to Chip and have Chip have different set of required props based on type? Problem is 1. I only assume this is possible in TS. 2. I'd want to review all other props. Id might not be the only one that is dependent on variant. Maybe the throwing the error in renderChip is still the best option here. |
|
RE #1: I'm not sure if it is possible in TS, that's why I mentioned throwing an error. But then again this will be a breaking change for people if they have choice and didn't have What if we pass down the variant to the Chip so that it can throw an error? |
|
I am thinking the least intrusive change would be to simply catch it in ChipSet's renderChip method. From my understanding, if the Chip is not a part of ChipSet it never matters, nor does it need to know about its variant. |
|
Ah...ya that makes sense...let's do that! |
|
ChipSet's renderChip method will now throw error if ((choice || filter || input) && !chip.props.id). I was a bit unsure about the unit tests (and spent way too much time on them). If you have a preferred solution they can be changed. |
|
@mgr34 I like those tests, but looks like they are failing. |
|
@moog16 Well if that isn't frustrating. They passed in my working copy, but sure enough when I check it out to clean dir it fails. Oddly enough it fails on things I haven't changed. For example here are the first two errors: I am going to have to investigate this a bit more. Unfortunately, I am not sure that I will be able to get to that today. |
|
That's odd...maybe if we just restart the tests |
|
Wait, how would I restart the tests? to be clear I ran edit: Ok, I checked out everything into yet a different clean directory, but first ran tests on rc0.10.0 and that worked. I switched to this branch and I received the same errors Travis is reporting. So I guess I am onto something. Not sure why they're failing exactly. (Investigating...) |
|
@moog16 Alright, these tests are now passing for me. Lets see what Travis has to say... edit: Ok, looking at this the following morning I see that |
|
@mgr34 don't commit the package-lock. Only commit it if you added something to package.json, which you did not. Looks like there is an issue with screenshots, I'll try to get that fixed. |
|
@moog16 - sounds good to me. I was a little surprised to see that it was modified. Thought it was worth noting. |
|
ya that happens a lot, especially if our environments are different (node version, npm version, etc). |
|
makes sense. :) |
Documentation claims id should be required, but was an optional property. Documentation mainly correct. A unique identifier is necessary for filter and choice variants. This should close issue #648 .
I signed it