fix: Add default onAction prop #382
Conversation
PropTypes define onAction property as optional, however in class its used unconditionally.
Pull Request Test Coverage Report for Build 1459
💛 - Coveralls |
src/index.test.js
Outdated
| const { tree } = t.context | ||
| const wrapper = mount(<DropdownTreeSelect id={dropdownId} data={tree} showDropdown="initial" />) | ||
| wrapper.find('i.fa-ban').simulate('click') | ||
| t.pass() |
There was a problem hiding this comment.
I'm not entirely sure what I need to check here 🤔
The test is only checking if the click event does not trigger error, therefore it pass.
Any suggestions what would be correct test for it otherwise?
There was a problem hiding this comment.
Well, there is literally no checks/assertions in this test. It adds no value. I'm not sure if there's a way to check for those type of warnings during a test run. If we can't find a way to ensure that a specific warning is not emitted, then I'd rather not have this test.
There was a problem hiding this comment.
I think one way we could check for this is spy console.warn or console.error (whichever is emitted by React) and then check that it doesn't get called.
You can remove your default prop and this test should fail - which would confirm that this approach works :)
There was a problem hiding this comment.
Well, because it's actual JavaScript TypeError, nothing to do with React itself, it already fails by default without any checks, if default prop is removed, like this:
index › notifies on action without onAction handler
....../javascript/react-dropdown-tree-select/src/index.js:216
215: onAction = (nodeId, action) => {
216: this.props.onAction(this.treeManager.getNodeById(nodeId), action)
217: }
Error thrown in test:
TypeError {
message: '_this.props.onAction is not a function',
}
I changed from pass() to spy for console error, but honestly I don't see any value in it.
Let me know if this acceptable now
There was a problem hiding this comment.
Well pass is of no value. This test actually checks and will warn in future if we break it again. I'll check it and merge (mostly looks good).
|
@all-contributors Please add @kYem for bug, code |
|
I've put up a pull request to add @kYem! 🎉 |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
@mrchief Is this going to get merged? |
|
Code Climate has analyzed commit 4d7a6a0 and detected 0 issues on this pull request. View more on Code Climate. |
|
🎉 This PR is included in version 2.3.7 🎉 The release is available on: Your semantic-release bot 📦🚀 |
* fix: Add default onAction prop PropTypes define onAction property as optional, however in class its used unconditionally. * fix: Add console error spy check Co-authored-by: Hrusikesh Panda <mrchief@users.noreply.github.com>
What does it do?
PropTypes define onAction property as optional, however in class its
used unconditionally. Trying to click the action without passing onAction handler cause type error.
Fixes #380
Type of change
Checklist: