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

Conversation

Projects
None yet
4 participants
@Chrissy
Copy link
Contributor

Chrissy commented Jun 21, 2018

Updates:

  • I'm gonna wait to move all of this to styled components until a follow-up. This pull is getting complicated.
  • I tried not to add/refactor too much js other than general housekeeping, but I did need to add in a min-height functionality since we use that feature a lot and svg doesn't support it.
  • I've found several bugs with our charts. One I fixed and the rest I will address in follow-up pulls. They are noted below.

Changes:

Our current bar charts do all sorts of crazy things if odd numbers are involved. This is because divs aren't subject to sub-pixel aliasing (or rather, they are but the browser doesn't seem to couple the antialiasing process with the precise layout):

2018-06-20_1720

SVGs are capable of sub-pixel aliasing and even have several different options to fit different goals. Best of all, they take layout precision into account much better during the aliasing process

image

Here is what a complex example looks like before and after:

example3

Circles also don't scale very well in the current charts:

square-circles

This converts our circles to svg so it scales correctly:

circle-circles

Performance is equal at worst, and might be a little better based on a quick look at the performance tab. There is the opportunity to emphatically improve performance in tabular list views by converting bars to a sprite with <use />.

As the charts get smaller and we move into sub-pixel layouts the improvements become more obvious:

image

image

image

image

todo:

  • Visually replicate old charts
  • Stacking functionality
  • Tooltip functionality
  • Test in different browsers
  • SVG Markers
  • Check performance
  • Check all views
  • Cleanup

other (existing) chart bugs for follow ups:

  • colors don't work on various settings charts anymore
  • several colors are no longer in the sentry brand
  • min-height is not factored into maximum chart height

@Chrissy Chrissy added the WIP label Jun 21, 2018

@ckj

This comment has been minimized.

Copy link
Member

ckj commented Jun 21, 2018

😍

@Chrissy Chrissy force-pushed the ref/issues/svg-bar-charts branch from 50156de to 90ec931 Sep 18, 2018

});
}
},
}

shouldComponentUpdate(nextProps, nextState) {
return !_.isEqual(this.props, nextProps);

This comment has been minimized.

@Chrissy

Chrissy Sep 19, 2018

Author Contributor

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.

This comment has been minimized.

@Chrissy

Chrissy Sep 19, 2018

Author Contributor

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

This comment has been minimized.

@billyvg

billyvg Sep 21, 2018

Member

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.

@Chrissy Chrissy removed the WIP label Sep 25, 2018

<rect
x={index * pointWidth + '%'}
width={pointWidth + '%'}
height="105%"

This comment has been minimized.

@Chrissy

Chrissy Sep 25, 2018

Author Contributor

The way we do stacked charts with min-heights right now is similar to this. we set a min-height in css and then it just stacks up and overflows the container.

A fancier and better version of this could apply the min-heights to a base set and then calculate everything based on that, but that seemed out of scope since I was really just trying to replicate the old chart behavior in svg.

const StyledSvg = styled('svg')`
width: calc(100% + ${p => p.gap}px);
height: 100%;
overflow: visible !important; /* overrides global declaration */

This comment has been minimized.

@Chrissy

Chrissy Sep 25, 2018

Author Contributor

we set overflow: hidden globally on all svgs 🙄

@@ -345,36 +345,39 @@
border-bottom: 1px dotted @trim;
}

.bar-chart figure a {
.bar-chart figure a,
.bar-chart figure g {

This comment has been minimized.

@Chrissy

Chrissy Sep 25, 2018

Author Contributor

this css will all go away, but I want to do it in a follow up.

@Chrissy Chrissy requested review from getsentry/app-frontend and ckj Sep 25, 2018

@ckj
Copy link
Member

ckj left a comment

😍😍😍

Visually everything looks great, save for a handful of diffs... Any idea what's going on to cause the markers in the sidebar to be on the first bar instead of the last?

https://percy.io/getsentry/sentry/builds/1035439/view/63270274/1280?browser=firefox&mode=diff

https://percy.io/getsentry/sentry/builds/1035439/view/63270225/1280?browser=firefox&mode=diff

@Chrissy

This comment has been minimized.

Copy link
Contributor Author

Chrissy commented Sep 25, 2018

@ckj yeah I've been looking at that too. When I compare my branch directly to master it is almost identical...so I'm hoping this is a percy thing. I will probably dig around a bit more before I mash merge just to make sure it's production safe.

@ckj

This comment has been minimized.

Copy link
Member

ckj commented Sep 25, 2018

@Chrissy tooltips are a bit off on the smaller charts
screen shot 2018-09-25 at 4 43 33 pm

@Chrissy

This comment has been minimized.

Copy link
Contributor Author

Chrissy commented Sep 25, 2018

@ckj good catch! that was added recently and should be an easy fix.

@Chrissy

This comment has been minimized.

Copy link
Contributor Author

Chrissy commented Sep 26, 2018

alrighty @ckj I addressed both of the bugs you found. I moved the circles into independent svgs that are moved using css, and then changed the bar graphs to stretch, so everything scales fluidly now both vertically and horizontally (but...you know...without the weird bugs).

Everything looks good in comparison to master, so once I get a js review, this should be ready to go.

},
}

getInterval = series => {

This comment has been minimized.

@Chrissy

Chrissy Sep 27, 2018

Author Contributor

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

import PropTypes from 'prop-types';
import moment from 'moment-timezone';
import _ from 'lodash';
import styled from 'react-emotion';
import cx from 'classnames';

This comment has been minimized.

@billyvg

billyvg Sep 27, 2018

Member

fyi react-emotion has a cx as well (and would prefer that moving forward)

This comment has been minimized.

@Chrissy

Chrissy Sep 27, 2018

Author Contributor

was just reading about this. good call.

}

getInterval = series => {
// TODO(dcramer): not guaranteed correct

This comment has been minimized.

@billyvg

This comment has been minimized.

@Chrissy

Chrissy Sep 27, 2018

Author Contributor

RIGHT?!

let totalY = point.y.reduce((a, b) => a + b);
let totalPct = totalY / maxval;
let prevPct = 0;
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),

This comment has been minimized.

@billyvg

billyvg Sep 27, 2018

Member

what does 99 mean here?

This comment has been minimized.

@Chrissy

Chrissy Sep 27, 2018

Author Contributor

(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.

This comment has been minimized.

@billyvg

billyvg Sep 28, 2018

Member

Also good for this to be in commentz


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((100.0 + this.props.gap + 0.1) / totalPoints, 2);

This comment has been minimized.

@billyvg

billyvg Sep 27, 2018

Member

What does 0.1 mean here?

This comment has been minimized.

@Chrissy

Chrissy Sep 27, 2018

Author Contributor

it keeps a very subtle white line from appearing on the right edge of the svg at certain sizes. I could remove it and nobody would notice. it happens with regularity in production and nobody seems to care.

This comment has been minimized.

@billyvg

billyvg Sep 27, 2018

Member

I would explain this in some code commentz

barClasses: ['chart-bar'],
};
},
minHeights: PropTypes.arrayOf(PropTypes.number),

This comment has been minimized.

@billyvg

billyvg Sep 27, 2018

Member

Can we document what minHeights does? It's not clear why it's an array of min heights.

This comment has been minimized.

@Chrissy

Chrissy Sep 27, 2018

Author Contributor

absolutely

This comment has been minimized.

@Chrissy

Chrissy Sep 27, 2018

Author Contributor

I also considered renaming it to minBarHeights which might help

Show resolved Hide resolved tests/js/spec/components/stackedBarChart.spec.jsx
@billyvg
Copy link
Member

billyvg left a comment

lgtm, I would add some inline comments for places where magic numbers are involved.

@Chrissy

This comment has been minimized.

Copy link
Contributor Author

Chrissy commented Oct 1, 2018

alrighty @billyvg @ckj everything is resolved. gonna check on Windows one more time and then merge.

@Chrissy Chrissy merged commit 0f863d7 into master Oct 1, 2018

6 checks passed

codecov/patch/javascript 89.09% of diff hit (target 60%)
Details
codecov/patch/python Coverage not affected when comparing 9a19525...8bc282c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/snyk - package.json (sentry) No new issues
Details
percy/sentry No visual changes since last approval
Details
security/snyk - package.json (sentry) No new issues
Details

@ckj ckj deleted the ref/issues/svg-bar-charts branch Oct 1, 2018

@ckj

This comment has been minimized.

Copy link
Member

ckj commented Oct 1, 2018

📈

@benvinegar

This comment has been minimized.

Copy link
Member

benvinegar commented Oct 2, 2018

💯 Huge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment