LineSeriesCanvas - onNearestXY not called#931
Conversation
|
|
|
Looks like lint is failing! |
|
will fix one moment |
mcnuttandrew
left a comment
There was a problem hiding this comment.
Thanks for making this PR! Lots of comments, but overall nice job!
| "lint": "eslint src tests showcase docs --ignore-pattern node_modules --ignore-pattern bundle.js", | ||
| "lint-styles": "stylelint src/styles/*.scss --syntax scss", | ||
| "test": "node --inspect node_modules/.bin/babel-node tests/index.js", | ||
| "test:windows": "babel-node --inspect ./tests/index.js", |
There was a problem hiding this comment.
i wasn't intended to remove test command just one i created for windows, not sure how this happen, i added my own because i cant "npm run test" on windows, i have error ( this one jeffrifwald/babel-istanbul#70 ) , same goes for lint i should add "linebreak-style": ["error", "windows"]" to make it work, probably tests wasn't meant to run on windows platform
There was a problem hiding this comment.
that's super weird! i don't really know what to do about it. Maybe file a bug and it can be looked into separately?
There was a problem hiding this comment.
ok i will create separate issue for this
| import {rgb} from 'd3-color'; | ||
| import * as d3Shape from 'd3-shape'; | ||
|
|
||
| import React from 'react'; |
There was a problem hiding this comment.
nit: include line after external imports and before internal imports
There was a problem hiding this comment.
ok will insert line under external imports, i imported react because it was throwing errors
| }; | ||
|
|
||
| test('LineSeriesCanvas: should be rendered', t => { | ||
| const k = Math.round((Math.random() + 1) * 5); |
There was a problem hiding this comment.
Couple of things here.
0. I super applaud writing tests for these components, I'd be dragging my feet on that for a long time
- when writing tests it is a bad practice to use random values as this can to inconsistent test behaviour.
- Generally the pattern that is followed in our tests is to write a basic props test (like this one) and then write tests against showcase components, that way the UI can be inspected if something is confused. Would you mind turning your second test into a showcase component (also with non random values)
There was a problem hiding this comment.
i just tried to create showcase but was unable to compile showcase build, i created issue for this, sorry :)
| <XYPlot width={300} height={300}> | ||
| { | ||
| [...Array(k).keys()] | ||
| .map(v => <LineSeriesCanvas |
There was a problem hiding this comment.
you can combine these props like
<LineSeries Canvas {...{...LINE_PROPS, onNearestXY: (value, {event}) => t.pass(`onNearestXY called for series # ${v}`)}} />That being said you don't have a ton of props in LINE_PROPS, it might just be clearer to have the object directly.
| import LineSeriesCanvas from 'plot/series/line-series-canvas'; | ||
|
|
||
| const LINE_PROPS = { | ||
| className: 'line-chart-example', |
There was a problem hiding this comment.
this classname doesnt do anything right? you can remove it
There was a problem hiding this comment.
i just copy paste props from some different test, will do
| const LINE_PROPS = { | ||
| className: 'line-chart-example', | ||
| color: '#12939a', | ||
| data: [ |
There was a problem hiding this comment.
copy paste, will remove
mcnuttandrew
left a comment
There was a problem hiding this comment.
I pulled down your branch and tested it locally and it looks good! On another read through I don't mind the test stuff as much. My one suggestion would be to either delete your showcase example or to include it in showcase/showcase-components/plots-showcase.js and showcase/index.js for consistency. Once you make either change I'll merge this in
|
done, sorry for late response! |
| @@ -0,0 +1,4 @@ | |||
| import LineSeriesCanvas from './plot/line-series-canvas'; | |||
There was a problem hiding this comment.
Would you mind putting your component in the main plot showcase instead? see https://github.com/uber/react-vis/blob/master/showcase/showcase-sections/plots-showcase.js
|
chef kissing finger emoji this looks great! I'll merge it as soon integration finishes |
|
I don't get why it's still pending, looking at the build it passes. I'm gonna merge |
|
thanks, hope it helps, if you will have time take a look on another issue i created (#942) related to webpack, its probably not reasonable to fix it but adding small hint into docs can help to some strangers like me :) |
* fix * fix lint * remove windows related commands * suggested fixes * fix showcase * remove windows lint rule * append showcase to index * fix * move showcase
* fix * fix lint * remove windows related commands * suggested fixes * fix showcase * remove windows lint rule * append showcase to index * fix * move showcase
Hey! This suppose to fix issue with motion callback not called for canvas line series. I also added 2 tests to solidify this functionality, please let me know if i can improve something, hope it helps. Have a nice day.