adding defaultChecked property and changing checked property to take a boolean #88
Conversation
|
I am okay with using @paf31 What are your thoughts on this? |
|
Yes, agreed, it'd be better to do this everywhere, although it will be a breaking change. |
|
True, it would be breaking. But it would fine-tune the props. I think it could be a good way to go. @foopq Would you possibly be willing to make this part of your PR? |
As in reduce the space of valid values? Yes, I think we probably should do it. As for a release, maybe we can wait a little since we need to make a breaking release for 0.10 soon anyway. Thanks for merging these. |
|
Yep, reduce the space of valid values. Sounds good on the release. And no On Tuesday, 11 October 2016, Phil Freeman notifications@github.com wrote:
|
|
I'm happy including more in this PR. I'll try to go through the properties tomorrow night and change the others that should be boolean. |
|
Wonderful, thank you! On Tuesday, 11 October 2016, Chris Mennie notifications@github.com wrote:
|
|
Using the signatures from DefinatelyTyped and the React supported attributes page, I've updated the properties here. |
|
Looks great! Thank you very much for doing all of this. I will take a closer look and merge this in today or tomorrow. |
| dateTime :: String -> Props | ||
| dateTime = unsafeMkProps "dateTime" | ||
|
|
||
| default :: Boolean -> Props | ||
| default = unsafeMkProps "defaultChecked" |
There was a problem hiding this comment.
Should the default prop map to defaultChecked? Just want to be sure.
There was a problem hiding this comment.
Yup, this looks like a bad merge. I'll fix shortly.
| integrity :: String -> Props | ||
| integrity = unsafeMkProps "integrity" | ||
|
|
||
| is :: String -> Props |
There was a problem hiding this comment.
I don't believe that this is an HTML element. From what I have seen, it pertains to polymer. Should this be included? Can someone be using polymer with React? If so, are there other attributes for polymer that should be added? Perhaps such attributes (if added) should be in a separate module or at least separated out like the non-standard attributes below.
| @@ -99,13 +99,22 @@ coords = unsafeMkProps "coords" | |||
| crossOrigin :: String -> Props | |||
| crossOrigin = unsafeMkProps "crossOrigin" | |||
|
|
|||
| dataAttr :: String -> Props | |||
There was a problem hiding this comment.
Is there a reference to this attribute? I was trying to determine what it does, but didn't find anything.
There was a problem hiding this comment.
For these attributes you're asking about, I'm going by what's listed on https://facebook.github.io/react/docs/tags-and-attributes.html. Some of them I'm unfamiliar with as well.
| key :: String -> Props | ||
| key = unsafeMkProps "key" | ||
|
|
||
| keyParams :: String -> Props |
There was a problem hiding this comment.
Is there a reference to this attribute? I just wanted to determine what it does.
| muted = unsafeMkProps "muted" | ||
|
|
||
| name :: String -> Props | ||
| name = unsafeMkProps "name" | ||
|
|
||
| noValidate :: String -> Props | ||
| nonce :: String -> Props |
There was a problem hiding this comment.
Also wanted to double check this attribute. I thought it might be for the script tag, but was not sure.
| start = unsafeMkProps "start" | ||
|
|
||
| step :: String -> Props | ||
| step = unsafeMkProps "step" | ||
|
|
||
| tabIndex :: String -> Props | ||
| summary :: String -> Props |
There was a problem hiding this comment.
This attribute is not supported in HTML5. Should we still include this here?
|
Thanks for the fix for I did a quick scan and noticed a few that we don't have. Would it be okay to add:
Also Thanks! |
|
Looks great! Thanks so much for this addition. |
|
My pleasure :) |
Taking an opinionated stand on #81 (other bindings have checked as a boolean, such as https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/react/react.d.ts).
Also adding defaultChecked as per https://facebook.github.io/react/docs/forms.html#default-value