Skip to content
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

ref(ui): svg-based bar charts #8802

Merged
merged 30 commits into from Oct 1, 2018
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
90ec931
random height
Chrissy Jun 7, 2018
3330874
rough working scalable version
Chrissy Sep 18, 2018
8ada768
perfect crop box
Chrissy Sep 18, 2018
e04e8d7
improve bar spacing to look identical to old spacing
Chrissy Sep 18, 2018
3edc908
working stax
Chrissy Sep 18, 2018
dae1ae7
only have a min height on the first bar
Chrissy Sep 18, 2018
4778688
remove createReactClass for posterity of the world
Chrissy Sep 18, 2018
c8cdbf5
use container: body so that tooltips can render outside the svg ns
Chrissy Sep 19, 2018
b3fdab6
cleanup and tests
Chrissy Sep 19, 2018
51bb905
patch more css hidden in weird nooks
Chrissy Sep 20, 2018
af04721
found a couple more bits of css
Chrissy Sep 20, 2018
b8ab38d
enable customization of things like min height and gap space since th…
Chrissy Sep 20, 2018
a9bbeaa
working markers
Chrissy Sep 21, 2018
1cb7422
use masking for fixed spaces between bars
Chrissy Sep 24, 2018
9d29796
add circles to svg and fix id bug with clip-path
Chrissy Sep 24, 2018
62c7d92
use proper gap in groupChart view
Chrissy Sep 24, 2018
2bca6db
perfect identicalness to current charts
Chrissy Sep 25, 2018
da4aeb5
fix min height for super short project stats graph
Chrissy Sep 25, 2018
a9135dd
show tooltip during any hover contact with bar height, empty or not
Chrissy Sep 25, 2018
703c508
fix min heights on group chart
Chrissy Sep 25, 2018
2ffb198
remove unecessary gap=2 stuff
Chrissy Sep 25, 2018
ddeb43e
add an extra 5% at the top of bar charts to prevent extra minheight c…
Chrissy Sep 25, 2018
ac7ec91
remove chart marker css
Chrissy Sep 25, 2018
60d673e
add params to 'bizarre case' code
Chrissy Sep 25, 2018
1219f8f
fix tooltip placement on smaller graphs
Chrissy Sep 26, 2018
cb8ece8
place circles as seperate svgs
Chrissy Sep 26, 2018
de6be6b
convert to percentage based gap widths because it is simple and with …
Chrissy Sep 26, 2018
4c0aa82
project stats gap
Chrissy Sep 26, 2018
593a722
add comments and use a stretch viewbox for safety reasons
Chrissy Oct 1, 2018
8bc282c
update comment to make more sense
Chrissy Oct 1, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
208 changes: 110 additions & 98 deletions src/sentry/static/sentry/app/components/stackedBarChart.jsx
@@ -1,17 +1,15 @@
import React from 'react';
import createReactClass from 'create-react-class';
import PropTypes from 'prop-types';
import moment from 'moment-timezone';
import _ from 'lodash';
import styled from 'react-emotion';

import Tooltip from 'app/components/tooltip';
import Count from 'app/components/count';
import ConfigStore from 'app/stores/configStore';

const StackedBarChart = createReactClass({
displayName: 'StackedBarChart',

propTypes: {
class StackedBarChart extends React.Component {
static propTypes = {
// TODO(dcramer): DEPRECATED, use series instead
points: PropTypes.arrayOf(
PropTypes.shape({
Expand Down Expand Up @@ -42,73 +40,39 @@ const StackedBarChart = createReactClass({
),
tooltip: PropTypes.func,
barClasses: PropTypes.array,
},

statics: {
getInterval(series) {
// TODO(dcramer): not guaranteed correct
return series.length && series[0].data.length > 1
? series[0].data[1].x - series[0].data[0].x
: null;
},

pointsToSeries(points) {
let series = [];
points.forEach((p, pIdx) => {
p.y.forEach((y, yIdx) => {
if (!series[yIdx]) {
series[yIdx] = {data: []};
}
series[yIdx].data.push({x: p.x, y});
});
});
return series;
},

pointIndex(series) {
let points = {};
series.forEach(s => {
s.data.forEach(p => {
if (!points[p.x]) {
points[p.x] = {y: [], x: p.x};
}
points[p.x].y.push(p.y);
});
});
return points;
},
},

getDefaultProps() {
return {
className: 'sparkline',
height: null,
label: '',
points: [],
series: [],
markers: [],
width: null,
barClasses: ['chart-bar'],
};
},
};

static defaultProps = {
className: 'sparkline',
height: null,
label: '',
points: [],
series: [],
markers: [],
width: null,
barClasses: ['chart-bar'],
};

constructor(props) {
super(props);

getInitialState() {
// massage points
let series = this.props.series;

if (this.props.points.length) {
if (series.length) {
throw new Error('Only one of [points|series] should be specified.');
}

series = StackedBarChart.pointsToSeries(this.props.points);
series = this.pointsToSeries(this.props.points);
}

return {
this.state = {
series,
pointIndex: StackedBarChart.pointIndex(series),
interval: StackedBarChart.getInterval(series),
pointIndex: this.pointIndex(series),
interval: this.getInterval(series),
};
},
}

componentWillReceiveProps(nextProps) {
if (nextProps.points || nextProps.series) {
Expand All @@ -118,31 +82,64 @@ const StackedBarChart = createReactClass({
throw new Error('Only one of [points|series] should be specified.');
}

series = StackedBarChart.pointsToSeries(nextProps.points);
series = this.pointsToSeries(nextProps.points);
}

this.setState({
series,
pointIndex: StackedBarChart.pointIndex(series),
interval: StackedBarChart.getInterval(series),
pointIndex: this.pointIndex(series),
interval: this.getInterval(series),
});
}
},
}

shouldComponentUpdate(nextProps, nextState) {
return !_.isEqual(this.props, nextProps);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this really necessary? seems like a) this would be close to the default behavior and b) it would be a confusing surprise to not re-render on state change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm genuinely asking tho (hence why I left it in)

Copy link
Member

Choose a reason for hiding this comment

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

Difference is shallow vs deep compare, agreed with (b).

Since we're not updating on state changes... the values that are currently in state, should not be in state. I would leave this in here though and open a followup for cleanups.

},
}

getInterval = series => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

none of these changed, i just moved them when I removed createClass.

// TODO(dcramer): not guaranteed correct
Copy link
Member

Choose a reason for hiding this comment

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

🧐

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RIGHT?!

return series.length && series[0].data.length > 1
? series[0].data[1].x - series[0].data[0].x
: null;
};

pointsToSeries = points => {
let series = [];
points.forEach((p, pIdx) => {
p.y.forEach((y, yIdx) => {
if (!series[yIdx]) {
series[yIdx] = {data: []};
}
series[yIdx].data.push({x: p.x, y});
});
});
return series;
};

pointIndex = series => {
let points = {};
series.forEach(s => {
s.data.forEach(p => {
if (!points[p.x]) {
points[p.x] = {y: [], x: p.x};
}
points[p.x].y.push(p.y);
});
});
return points;
};

use24Hours() {
let user = ConfigStore.get('user');
let options = user ? user.options : {};
return options.clock24Hours;
},
}

floatFormat(number, places) {
let multi = Math.pow(10, places);
return parseInt(number * multi, 10) / multi;
},
}

timeLabelAsHour(point) {
let timeMoment = moment(point.x * 1000);
Expand All @@ -158,13 +155,13 @@ const StackedBarChart = createReactClass({
nextMoment.format(format) +
'</span>'
);
},
}

timeLabelAsDay(point) {
let timeMoment = moment(point.x * 1000);

return `<span>${timeMoment.format('LL')}</span>`;
},
}

timeLabelAsRange(interval, point) {
let timeMoment = moment(point.x * 1000);
Expand All @@ -179,12 +176,12 @@ const StackedBarChart = createReactClass({
nextMoment.format(format) +
'</span>'
);
},
}

timeLabelAsFull(point) {
let timeMoment = moment(point.x * 1000);
return timeMoment.format('lll');
},
}

getTimeLabel(point) {
switch (this.state.interval) {
Expand All @@ -197,7 +194,7 @@ const StackedBarChart = createReactClass({
default:
return this.timeLabelAsRange(this.state.interval, point);
}
},
}

maxPointValue() {
return Math.max(
Expand All @@ -206,7 +203,7 @@ const StackedBarChart = createReactClass({
.map(s => Math.max(...s.data.map(p => p.y)))
.reduce((a, b) => a + b, 0)
);
},
}

renderMarker(marker) {
let timeLabel = moment(marker.x * 1000).format('lll');
Expand All @@ -219,14 +216,14 @@ const StackedBarChart = createReactClass({

return (
<Tooltip title={title} key={key} tooltipOptions={{html: true, placement: 'bottom'}}>
<a className={className} style={{height: '100%'}}>
<a data-test-id="chart-column" className={className} style={{height: '100%'}}>
<span>{marker.label}</span>
</a>
</Tooltip>
);
},
}

renderTooltip(point, pointIdx) {
renderTooltip = (point, pointIdx) => {
let timeLabel = this.getTimeLabel(point);
let totalY = point.y.reduce((a, b) => a + b);
let title =
Expand All @@ -245,26 +242,30 @@ const StackedBarChart = createReactClass({
}
});
return title;
},
};

renderChartColumn(point, maxval, pointWidth) {
renderChartColumn(point, maxval, pointWidth, index, totalPoints) {
let totalY = point.y.reduce((a, b) => a + b);
let totalPct = totalY / maxval;
let prevPct = 0;
let space = 36 / totalPoints;
let pts = point.y.map((y, i) => {
let pct = totalY && this.floatFormat(y / totalY * totalPct * 99, 2);
let pct = Math.max(
totalY && this.floatFormat(y / totalY * totalPct * 99, 2),
Copy link
Member

Choose a reason for hiding this comment

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

what does 99 mean here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(takes a long, deep breath...)

As best I can tell the "99" is used to reserve a little extra space for the min-heights that were previously applied to bars with css (now applied with the prop). this didn't really work because 1% isn't always 1px and sometimes (like with issue release charts) the min-height is 4+ pixels.

However none of the charts have overflow: hidden so if the bars exceed 100% height it doesn't matter (so long as visually it doesn't look weird)

I kept the 99 in there to keep things similar to what they were before. Truthfully, the charts have never been that accurate lol. We could do 100 and nobody would notice.

Copy link
Member

Choose a reason for hiding this comment

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

Also good for this to be in commentz

i == 0 ? 1.5 : 0
);
let pt = (
<span
<rect
key={i}
x={index * pointWidth}
y={100 - pct - prevPct}
width={pointWidth - space}
height={pct}
fill={this.state.series[i].color}
className={this.props.barClasses[i]}
style={{
height: pct + '%',
bottom: prevPct + '%',
backgroundColor: this.state.series[i].color || null,
}}
>
{y}
</span>
</rect>
);
prevPct += pct;
return pt;
Expand All @@ -277,19 +278,17 @@ const StackedBarChart = createReactClass({
<Tooltip
title={tooltipFunc(this.state.pointIndex[pointIdx], pointIdx, this)}
key={point.x}
tooltipOptions={{html: true, placement: 'bottom'}}
tooltipOptions={{html: true, placement: 'bottom', container: 'body'}}
>
<a className="chart-column" style={{width: pointWidth, height: '100%'}}>
{pts}
</a>
<g data-test-id="chart-column">{pts}</g>
</Tooltip>
);
},
}

renderChart() {
let {pointIndex, series} = this.state;
let totalPoints = Math.max(...series.map(s => s.data.length));
let pointWidth = this.floatFormat(100.0 / totalPoints, 2) + '%';
let pointWidth = this.floatFormat(101.0 / totalPoints, 2);

let maxval = this.maxPointValue();
let markers = this.props.markers.slice();
Expand All @@ -309,12 +308,14 @@ const StackedBarChart = createReactClass({
});

let children = [];
points.forEach(point => {
points.forEach((point, index) => {
while (markers.length && markers[0].x <= point.x) {
children.push(this.renderMarker(markers.shift()));
}

children.push(this.renderChartColumn(point, maxval, pointWidth));
children.push(
this.renderChartColumn(point, maxval, pointWidth, index, totalPoints)
);
});

// in bizarre case where markers never got rendered, render them last
Expand All @@ -324,7 +325,7 @@ const StackedBarChart = createReactClass({
});

return children;
},
}

render() {
let {className, style, height, width} = this.props;
Expand All @@ -337,10 +338,21 @@ const StackedBarChart = createReactClass({
<Count value={maxval} />
</span>
<span className="min-y">0</span>
<span>{this.renderChart()}</span>
<StyledSvg
shapeRendering="geometricPrecision"
viewBox={'0 0 99.9 99.9'}
preserveAspectRatio="none"
>
{this.renderChart()}
</StyledSvg>
</figure>
);
},
});
}
}

const StyledSvg = styled('svg')`
width: 100%;
height: 100%;
`;

export default StackedBarChart;
8 changes: 6 additions & 2 deletions src/sentry/static/sentry/less/components/charts.less
Expand Up @@ -15,13 +15,16 @@
cursor: default;

&:hover {
> span {
> span,
> rect {
background: @purple;
fill: @purple;
border-color: transparent;
}
}

> span {
> span,
> rect {
display: block;
position: absolute;
bottom: 0;
Expand All @@ -35,6 +38,7 @@
min-height: 1px;

background: @gray-lighter;
fill: @gray-lighter;
}
}

Expand Down