Bug fixes #258 and #257 #259
Conversation
With 1.3.0 Events Drop and D3 5.7.0, the global in index.js was undefined. Therefore adding it to the parameters. If there is a better way of doing this please let me know and I will modify it. should solve the following issues: marmelab#258 and marmelab#244
Fix is taken from issue marmelab#257 and was suggested by @christianmerour Adding this due to bug fixes, this fix is also related to marmelab#241 and marmelab#239.
| .on('click', onClick) | ||
| .on('mouseover', onMouseOver) | ||
| .on('mouseout', onMouseOut) | ||
| .merge(drops) | ||
| .attr('r', dropRadius) |
There was a problem hiding this comment.
Can you try add a test to ensure it won't break again? Probably by rendering a chart, updating its data and check both radius and color?
There was a problem hiding this comment.
Will have a look, might be worth if @christianmerour could have a look as he found the solution. Haven't done too much testing in JS, but will see if I can get it sorted.
There was a problem hiding this comment.
@jpetitcolas from the issue #241 it seems that in order to test this, the zoom needs to be invoked on the chart. I don't really know how this can be done externally without writing a function that does it. However, the issue does describe the bug in detail.
Could maybe add enough data and then somehow zoom in and out, then check the data that was outside the viewable area? Could maybe dispatch a zoom event (the custom dispatch ones) and then reset the zoomIdentity, which I think is doable. But I doubt I have time to mess around with this unfortunately.
Edit*
Forgot to add that I did try to create a chart, then update the data and check if the radius and colour were the same (it was the same). So I wasn't able to reproduce the bug that way.
There was a problem hiding this comment.
Indeed ,forgot that testing zoom was not done for now. And not sure on how to do it. Merged! :)
Fixes issues where global is undefined and radius and colour flickering after merge.
This PR also fixes more than the two issues in the title, which can be found in the specific commit message.
Thanks to @christianmerour for his fix on #257, thought I might include it in this PR as it is related to bug fixes.
Prettier did some formatting on index.js.
Please modify as seen fit.
edit:
Fixes #257, fixes #258, fixes #244, fixes #241 and fixes #239