Setting type attribue with setAttribute function#2147
Conversation
JoviDeCroock
left a comment
There was a problem hiding this comment.
we usually add a test to assert if this is done correctly to prevent future regressions
Sorry. I'll add test for this too. |
|
No need to say sorry, just notifying thanks for tackling this |
|
That's awesome, thank you so much for the PR 👍 🎉 For tests you can take a look at the existing ones. We have a few where we check if an operation throws an error via |
b6c328d to
f6b6319
Compare
| render(<input type="date" />, scratch); | ||
| expect(scratch.childNodes).to.have.length(1); | ||
| expect(scratch.childNodes[0].nodeName).to.equal('INPUT'); | ||
| expect(scratch.childNodes[0].type).to.equal('date'); |
There was a problem hiding this comment.
I think we should test something that more closely resembles the original bug report. The fix in this PR specifically addresses the case where setting input.date would throw an error in IE11. The test case should reflect only that. On our CI our test suite will be run in various browsers, including IE11. So we only need to check if it throws or not.
it('should not throw in IE11 with type date', () => {
expect(() => render(<input type="date" />, scratch)).to.not.throw();
});f6b6319 to
55143e6
Compare
marvinhagemeister
left a comment
There was a problem hiding this comment.
Awesome, the changes look good to me 👍 💯 Again, thank you so much for the PR 🙌
|
Thanks for the help @marvinhagemeister @JoviDeCroock you guys are awesome !!! |
JoviDeCroock
left a comment
There was a problem hiding this comment.
Thanks for the PR, you're awesome 💯
This is done to prevent crash in IE11 as mentioned in this issue #2110