Skip to content
This repository was archived by the owner on Jun 4, 2024. It is now read-only.

prop type check #108

Closed
wants to merge 4 commits into from
Closed

prop type check #108

wants to merge 4 commits into from

Conversation

byronz
Copy link
Contributor

@byronz byronz commented Apr 2, 2019

@alexcjohnson @chriddyp I have a concern about the current state of our defined PropTypes,

one example is draggable: PropTypes.string, but per HTML reference it's an enumerated attribute with {true, false, auto}.

we don't have another useful attribute inputmode, which is also an enumerated list, and it's a good demo case for <textarea> and

this fixes #107

@byronz byronz changed the title WIP - 107 prop type check #107 prop type check Apr 2, 2019
@byronz
Copy link
Contributor Author

byronz commented Apr 2, 2019

@plotly/dash-core a quick glimpse of generate-component.js and extract-attributes.js, and also found a closed issue create by ryan, I think it makes sense to revisit the accuracy of the prop types definition for global attributes.

@byronz byronz requested review from alexcjohnson and chriddyp April 2, 2019 20:12
@chriddyp
Copy link
Member

chriddyp commented Apr 2, 2019

I think it makes sense to revisit the accuracy of the prop types definition for global attributes.

👍 . I just found this data in a structured format here: https://github.com/iandevlin/html-attributes, seems like we could use that in our component generation script.

@chriddyp
Copy link
Member

chriddyp commented Apr 2, 2019

also might need to evaluate how we deal with booleans too. I think we should accept them and coerce to "true".

not sure what to do about False - do we compile to "false" or do we omit? in the html spec, for a value to be false, it needs to be omitted rather than set to "false"

https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes#Boolean_Attributes:

Some content attributes (e.g. required, readonly, disabled) are called boolean attributes. If a boolean attribute is present, its value is true, and if it’s absent, its value is false.

However, I believe React will just ignore it now: https://reactjs.org/blog/2017/09/08/dom-attributes-in-react-16.html & https://reactjs.org/docs/jsx-in-depth.html#props-default-to-true
image

@byronz
Copy link
Contributor Author

byronz commented Apr 2, 2019

the draggable example itself is also tricky, if you don't set, it's auto, when you set it, must be true and false, so a simple enum list might not be ideal

@byronz byronz changed the title #107 prop type check prop type check Apr 2, 2019
@alexcjohnson
Copy link
Collaborator

@byronz can we close this PR (and issue #107), since plotly/dash-renderer#100 does this for all components?

@byronz byronz closed this May 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add props type check for html components
3 participants