-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(Input): remove support for children content in <textarea/> #927
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need additional check to ensure the value of the textarea contains "Yo!"
src/__tests__/Input.spec.js
Outdated
it('should render without children when type is "textarea"', () => { | ||
const wrapper = shallow(<Input type="textarea">Yo!</Input>); | ||
|
||
expect(wrapper.text()).toBe(''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the test be checking to ensure that 'Yo!' is included inside the textarea. This test doesn't cover that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, that is how it will work (and react will spit out a warning), but in react 17 it will not work. React wants people to use value
/defaultValue
even for <textarea>
, making <textarea />
a void element.
This becomes a breaking change for reactstrap now, ahead of the breaking change react 17 will force us to make. It simplifies things a little it makes <textarea>
and <input>
basically the same.... which IMHO, HTML should have done <input type="textarea" />
(I wonder why they didn't... I'll have to Google)
Edit
Found https://stackoverflow.com/questions/5637326/why-isnt-textarea-an-inputtype-textarea which seems to suggest it was because someone suggested it before W3C was a thing, but another answer makes a little more sense; basically: <input/>
is void, using value
to set the value, but if you had a multi-line value, value
would need to encode the linebreaks and such, so a non-void element, <textarea>
was created.
src/Input.js
Outdated
@@ -93,6 +93,10 @@ class Input extends React.Component { | |||
attributes.type = type; | |||
} | |||
|
|||
if (type === 'textarea') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we may want to do something a little more restrictive. It looks like the only input which allows children is <select>
(and plaintext
/staticInput
). Maybe we should do something like
if (!(plaintext || staticInput || type === 'select' || typeof tag !== 'string')) {
attributes.children = null;
}
After though, I also added typeof tag !== 'string'
as if the user is passing another component to render (via the tag
prop), we should just let them do what they want as they may render a <select>
or something else which allows children. (similar use-case as it being in the if
condition above)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
your last expression will be typeof tag === 'string'
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be typeof tag !== 'string'
since we negate the entire condition.
For instance, if typeof tag
is "function", then typeof tag !== 'string'
will be true
, which would get negated to become false
which skip the code block and keep children.
It can be rewritten to make it more clear:
if (!plaintext && !staticInput && type !== 'select' && typeof tag === 'string') {
attributes.children = null;
}
plaintext/staticInput, type="select", and any tag which is not a string (or it is the string "select") at the only inputs which should be allowed to have children.
according to #871