Skip to content
This repository has been archived by the owner on Feb 19, 2022. It is now read-only.

(pre-release) Improvement/react art #97

Closed
wants to merge 12 commits into from

Conversation

boygirl
Copy link
Contributor

@boygirl boygirl commented Jan 28, 2016

cc/ @dandelany

This PR switches to react-art

This PR relies on my fork of react-art until this issue / PR lands reactjs/react-art#83

depends on FormidableLabs/victory-label#32

I also decided to punt on integration tests for a moment. Testing canvas elements is not working out nicely. I want to add enzyme to help out, but I'm bumping up against this issue enzymejs/enzyme#47. I'm going to talk with @kenwheeler and @coopy about a strategy.

@dandelany
Copy link
Contributor

Looks good! 👍 Pretty cool how easy it was to drop this in, I figured it would require a lot more work! Pulled & tried the demo page, all seems to work fine.

Re: integration tests, one possibility would be to write something that makes it easy to check the color of a given pixel in the canvas, since we're already doing a full render in PhantomJS instead of just checking HTML. Then we could do stuff like ensuring that the pixel where the corner of the bar is expected to be is actually the bar color, but the pixel above it is white. This gives us better assurances than the current unit tests - currently we're just checking that we have the right number of bars, while this would actually test that they are where they're supposed to be.

@boygirl
Copy link
Contributor Author

boygirl commented Jan 28, 2016

What I really would like is screenshot / image diffing, but I haven't been able to find a good solution for that. I guess we could write a little wrapper for rendering a saved image to canvas and then getImageData each one? https://github.com/tcorral/IM.js? https://github.com/Huddle/Resemble.js?

@dandelany
Copy link
Contributor

That's a good idea. Those libraries look interesting, if we used one of those we may not even have to render the saved image to canvas.

@boygirl boygirl changed the title Improvement/react art (pre-release) Improvement/react art Jan 28, 2016
@boygirl
Copy link
Contributor Author

boygirl commented Jan 28, 2016

cc/ @baer if you want to test this out locally you will need to npm link victory-label from the improvement/react-art branch, and npm link victory-util from the data-accessors branch. Sorry for all the overhead. git urls not currently working with builder.

@@ -2,7 +2,7 @@
import _ from "lodash";
import React from "react";
import {VictoryBar} from "../src/index";
import {VictoryChart} from "victory-chart";
// import {VictoryChart} from "victory-chart";
Copy link

Choose a reason for hiding this comment

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

Is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I haven't updated victory-chart to work with react-art yet, so that is all temporarily broken

@boygirl
Copy link
Contributor Author

boygirl commented Jan 29, 2016

react work is postponed

@boygirl boygirl closed this Jan 29, 2016
@boygirl boygirl deleted the improvement/react-art branch November 9, 2016 23:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants