-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Enhancement] Create factory for histogram and line chart, add brush handle to range brush #1274
Conversation
} | ||
} | ||
`; | ||
function HistogramPlotFactory() { |
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.
nit: missing new line
const domain = useMemo(() => [histogram[0].x0, histogram[histogram.length - 1].x1], [ | ||
histogram | ||
]); | ||
const dataId = Object.keys(histogram[0]).filter(k => k !== 'x0' && k !== 'x1')[0]; |
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.
you can use useMemo here too
fill: ${props => props.theme.textColor}; | ||
} | ||
`; | ||
const StyledHint = styled.div` |
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.
empty line missing
|
||
const HintContent = ({x, y, format}) => ( | ||
<StyledHint> | ||
<div className="hint--x">{format(x)}</div> |
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.
could you pass the result of format(x) as input of this component?
<div className="hint--x">{format(x)}</div> | ||
<div className="row">{y}</div> | ||
</StyledHint> | ||
); |
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.
new empty line
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.
just few comments
</StyledHint> | ||
); | ||
const MARGIN = {top: 0, bottom: 0, left: 0, right: 0}; | ||
function LineChartFactory() { |
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.
new empty line
src/components/common/range-brush.js
Outdated
const h = 39; | ||
const w = 4.5; | ||
const y = (props.height - h) / 2; | ||
// return `M0,57.3c2.5,0,4.5,2,4.5,4.5v29.3c0,2.5-2,4.5-4.5,4.5V57.3z` |
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.
remove commented lines
hintFormatter = createSelector(this.domainSelector, domain => | ||
getTimeWidgetHintFormatter(domain) | ||
RangePlotFactory.deps = [RangeBrushFactory, HistogramPlotFactory, LineChartFactory]; | ||
export default function RangePlotFactory(RangeBrush, HistogramPlot, LineChart) { |
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.
empty line
const wrapper = mountWithTheme(<RangePlot {...props} />); | ||
}, 'Show not fail without histogram'); | ||
|
||
// cant test D3 in jsDom for now |
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.
delete commented lines.
is testing only for render failures enough for this component?
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.
for now yea, better than nothing, we can't test d3 with jsDom at all, unless we created a bunch of utils for that component and only testing utils
Signed-off-by: Shan He <heshan0131@gmail.com>
brush interaction improve histogram Signed-off-by: Shan He <heshan0131@gmail.com> histogram plot tweaks Signed-off-by: Shan He <heshan0131@gmail.com>
Signed-off-by: Shan He <heshan0131@gmail.com>
Signed-off-by: Shan He <heshan0131@gmail.com>
dff426b
to
3e62f43
Compare
const domain = useMemo(() => [histogram[0].x0, histogram[histogram.length - 1].x1], [ | ||
histogram | ||
]); | ||
const dataId = Object.keys(histogram[0]).filter(k => k !== 'x0' && k !== 'x1')[0]; |
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's very cheap operation, and the return value is a string, it's probably more expensive to memorize it than just call it
Signed-off-by: Shan He <heshan0131@gmail.com>
improve histogram brush interaction
Signed-off-by: Shan He heshan0131@gmail.com