-
Notifications
You must be signed in to change notification settings - Fork 1
Conversation
Doing a great job here @haxxmaxx, but can we have it in multiple PR's so it will be easier to check? |
src/utils/selections.js
Outdated
@@ -10,7 +10,7 @@ export default { | |||
if (node.data.isLocked) { | |||
return; | |||
} | |||
if (!selections.api.isActive() || !selectionState) { | |||
if (!selections.api.isActive() || !selectionState) { // selectioState !== [] ? |
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.
@Caele Are we suppose to check if selectionState is emtpy?
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.
This will have some merge conflicts with the changes on the zooming though
select(node, selectionObj, selectionState); | ||
expect(selectionObj.setState).to.not.have.been.called; | ||
}); | ||
it('should early return if elemNo is negative', () => { |
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.
Different test case
select(node, selectionObj, selectionState); | ||
expect(selectionObj.setState).to.not.have.been.called; | ||
}); | ||
it('should call begin sand setState to node id', () => { |
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.
Typo
it('should add a warning for max data', () => { | ||
const matrix = [[{ qText: '007', qElemNumber: 0 }, { qText: '-' }]]; | ||
const node = createNodes(matrix, [], 'max-data-limit', null, translator); | ||
expect(node.warn).to.eql(['Object.OrgChart.MaxData']); |
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.
Does this function work?
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.
equal is equivalent to ===
, which would return false, since it is not the same instance of the array. So eql
checks the contents instead, kind of... https://stackoverflow.com/questions/36798993/what-is-the-difference-between-equal-and-eql-in-chai-library
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.
Ah nice, I have been using deep equal I think :)
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.
It is very similar, didn't really understand the difference when I read about it
Yeah I think that's good. After this coverage push, we should try to update the tests to our own changes 😉 |
Hard to disagree with that |
Pausing here so this wont get too big.
Todo in next PR:
tooltips
transform
tree-utils
position
render
andindex
are so much d3 and nebula so they are overkill to mock IMO.