Skip to content
3 changes: 3 additions & 0 deletions demo/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,14 @@ import React from 'react'

import TimeSeriesContainer from './ts-container'
import ScatterPieContainer from './sp-container'
import ScatterBasicContainer from './sb-container'


const App = () => (
<React.Fragment>
<TimeSeriesContainer />
<ScatterPieContainer />
<ScatterBasicContainer />
</React.Fragment>
)

Expand Down
78 changes: 78 additions & 0 deletions demo/sb-container.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
import React from 'react'
import { ScatterBasic } from '../src/visualizations'
import { GlyphDot } from '@vx/glyph'

const data = [{'label': 'Education', 'value_total': 0}, {'label': 'Visual and Performing Arts', 'value_total': 0}, {'label': 'Humanities', 'value_total': 0}, {'label': 'Sociology and Law', 'value_total': 15}, {'label': 'Business and PA', 'value_total': 10}, {'label': 'Phys Ed', 'value_total': 0}, {'label': 'Math, Comp and InfoSci', 'value_total': 0}, {'label': 'Engineering', 'value_total': 10}, {'label': 'Agriculture, NatRes', 'value_total': 0}, {'label': 'Health', 'value_total': 25}, {'label': 'Security', 'value_total': 0}, {'label': 'Other', 'value_total': 0}]

const scatterData = data.map((d) => {
return {
fieldName: d.label,
value: d.value_total,
}
})


class ScatterBasicContainer extends React.Component {
constructor(props) {
super(props)
this.state = {
fill: null,
radius: 4,
view: 'overview',
}
}

onPointHoverEnter = () => {
this.setState({
fill: '#b3ffff',
radius: (this.state.radius || 0) + 2,
})
}

onPointHoverLeave = () => {
this.setState({
fill: null,
radius: 4,
})
}

onPointClick = () => {
/**
* bring up modal for pie chart breakdown
*/
this.setState({
view: 'detail',
})
}

render() {
/** draw a scattered point */
const drawAScatter = (left, top) => {
return (
<GlyphDot
key={Math.random()}

@brianbancroft brianbancroft Sep 12, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is more commentary, and it's more intended for @woozyking. Please don't see this as a request for change at this time. I've just since run into a relevant piece in the React docs, which might be worth bringing to light. This is certainly not a critique of the work being done at this time!

The reason why I brought up math.random() as a key being a bad idea is that it causes issues with the process of reconciliation, where React tries to determine whether a subset of a component should be updated, or whether an entire component should be rendered.

The end of the page I've linked to seems to suggest that when employing math.random() as a key, there is a much higher chance that the element in question will be rebuilt completely, rather than partially.

If that's what is intended the context of this code, I'm not sure. d3 and charts being what they are, maybe stripping down and rebuilding is exactly what we want here.

https://reactjs.org/docs/reconciliation.html

@woozyking woozyking Sep 12, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think both @y629wang and yourself have mixed this with the case from the last PR #4 (comment), where it did make sense and the intent was to re-render the tooltip to take advantage of the dynamically computed boundary offered by vx.

From the look of this case, there's no reason to use Math.random() as key.

cx={left}
cy={top}
stroke='#008080'
fill='#b3ffff'
strokeWidth={1}
r={4}
/>
)
}

return (
<div style={{'height': '400'}}>
<h3 style={{textAlign: 'center'}}>Basic Scatter Plot</h3>
<ScatterBasic
drawAScatter={drawAScatter}
data={scatterData}
yAxisLabel='Population'
xAxisLabel='Program'
/>
</div>
)
}
}

export default ScatterBasicContainer
1 change: 1 addition & 0 deletions src/visualizations/index.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
export { default as TimeSeries } from './time-series'
export { default as Breakdown } from './breakdown'
export { default as ScatterPie } from './scatter-pie'
export { default as ScatterBasic } from './scatter-basic'
186 changes: 186 additions & 0 deletions src/visualizations/scatter-basic.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,186 @@
import React from 'react'
import PropTypes from 'prop-types'
import {
Scatter,
Axes,
// PieDonut,
Markers,
HotZone,
} from '../parts'

import { withParentSize } from '@vx/responsive'
import { withTooltip, TooltipWithBounds } from '@vx/tooltip'

import { numTicksForHeight, numTicksForWidth } from '../utils/responsive'
import { scaleLinear, scaleBand } from '@vx/scale'
import { extent } from 'd3-array'
import { Group } from '@vx/group'
// import { GlyphDot } from '@vx/glyph'
import { localPoint } from '@vx/event'

const ScatterBasic = ({
// withParentSize
parentWidth: width,
parentHeight: height,
// withToolTip
showTooltip,
hideTooltip,
tooltipOpen,
tooltipLeft,
tooltipTop,
tooltipData,
// own props
data,
margin,
yAxisLabel,
xAxisLabel,
drawAScatter,
}) => {

const xMax = width - margin.left - margin.right
const yMax = height - margin.bottom - margin.top

const kGetter = (obj) => obj.fieldName
const vGetter = (obj) => obj.value

const xScale = scaleBand({
domain: data.map(kGetter),
range: [0, xMax],
clamp: true,
})

const yScale = scaleLinear({
domain: extent(data, vGetter),
range: [yMax, 0],
clamp: true,
})

const renderAxes = () => {
return (
<Axes
showGrid={false}
xMax={xMax}
yMax={yMax}
left={{
top: 0,
left: 0,
scale: yScale,
numTicks: numTicksForHeight(height),
label: yAxisLabel,
}}
bottom={{
top: yMax,
left: 0,
scale: xScale,
numTicks: numTicksForWidth(width),
label: xAxisLabel,
}}
/>
)
}

// eslint-disable-next-line

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to use linter disable (and frankly we should minimize the use of it), since you're trying to handle the case of unused variable but the unused variable is a positional argument of the function handler -- simply remove it:

const scatterShape = (d) => { }

will do

const scatterShape = (d, _) => {
const bw = xScale.bandwidth() / 2
const left = xScale(kGetter(d)) + bw
const top = yScale(vGetter(d))

return drawAScatter(left, top)
}

const renderMarkers = () => (
tooltipOpen && (
<Markers
x={tooltipLeft}
y={tooltipTop}
xMax={xMax}
yMax={yMax}
/>
)
)

const renderToolTip = () => (
tooltipOpen && tooltipData && (
<TooltipWithBounds
left={tooltipLeft}
top={margin.top}>
{kGetter(tooltipData)}: {vGetter(tooltipData)}
</TooltipWithBounds>
))

const handleTooltip = ({ data, event }) => {
const point = localPoint(event)
const step = xScale.step()
const index = Math.round((point.x - margin.left) / step)

if (index < data.length && index >= 0) {
const pointData = data[index]
const bw = xScale.bandwidth() / 2
const tip = {
tooltipData: pointData,
tooltipLeft: xScale(kGetter(pointData)) + bw,
tooltipTop: yScale(vGetter(pointData)),
}
showTooltip(tip)
}
}

const renderTooltipTrigger = () => (
<HotZone
width={xMax}
height={yMax}
data={data}
onMouseMove={(data) => (event) => handleTooltip({ data, event })}
onMouseLeave={() => () => { hideTooltip() }}
/>
)

return (
<React.Fragment>
<svg width={width} height={height}>
<Group
left={margin.left}
bottom={margin.bottom}
top={margin.top}
right={margin.right}>
{renderMarkers()}
{renderAxes()}
{renderTooltipTrigger()}
<Scatter scatterShape={scatterShape} data={data}/>
</Group>
</svg>
{renderToolTip()}
</React.Fragment>
)
}

ScatterBasic.propTypes = {
// withParentSize
parentWidth: PropTypes.number.isRequired,
parentHeight: PropTypes.number.isRequired,
// withTooltip
showTooltip: PropTypes.func.isRequired,
hideTooltip: PropTypes.func.isRequired,
tooltipOpen: PropTypes.bool.isRequired,
tooltipTop: PropTypes.number,
tooltipLeft: PropTypes.number,
tooltipData: PropTypes.any,
// required
data: PropTypes.array.isRequired,
drawAScatter: PropTypes.func.isRequired,
yAxisLabel: PropTypes.string.isRequired,
xAxisLabel: PropTypes.string.isRequired,
// optional
margin: PropTypes.object,
}

ScatterBasic.defaultProps = {
margin: {
left: 100,
top: 20,
bottom: 50,
right: 100,
},
}

export default withParentSize(withTooltip(ScatterBasic))
1 change: 0 additions & 1 deletion src/visualizations/scatter-pie.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import {
Axes,
PieDonut,
NoSpace,
// Markers,
} from '../parts'
import { withParentSize } from '@vx/responsive'
import { withTooltip, TooltipWithBounds } from '@vx/tooltip'
Expand Down