Skip to content
This repository has been archived by the owner on Sep 17, 2021. It is now read-only.

Enhanced pie chart multicolor support, jest tests, & updated example sync util #36

Merged
merged 7 commits into from
Jan 6, 2017

Conversation

marzolfb
Copy link
Contributor

@marzolfb marzolfb commented Dec 21, 2016

  • Added support for using multiple colors on Pie chart as discussed in Solution for issue #5 about custom colors #22
    • There are now 3 ways to specify chart colors (listed in order or precedence):
      • Via a color property on the object of each item in the data array
      • Via props.palette (existing way to do it)
      • Via props.options.palette (added for consistency in handling of other props.options values for charts)
  • For pie chart specifically, added a new way to specify chart options/properties directly as props vs using the existing props.options approach
  • Added our first Jest snapshot tests for the project - this is really just a starting point to show it can be done
    • Tests compare runtime snapshots vs snapshot data stored in repo to provide some safety that unintended changes aren't being introduced
    • Modified circleci config to kick off test execution on a push to the repo
  • Removed util/watchman-update-example-src utility and replaced it with a new sync-rnpc script in the example directory (Approach borrowed from the example in react-native-router-flux)

@marzolfb
Copy link
Contributor Author

One more thing to point out. If you try to run npm test on these changes (or if you look at the circleci npm test output), you will get a ton of warnings like this:

jest-haste-map: duplicate manual mock found:
  Module name: ErrorUtils
  Duplicate Mock path: /home/ubuntu/react-native-pathjs-charts/node_modules/react-native/node_modules/fbjs/lib/__mocks__/ErrorUtils.js
This warning is caused by two manual mock files with the same file name.
Jest will use the mock file found in: 
/home/ubuntu/react-native-pathjs-charts/node_modules/react-native/node_modules/fbjs/lib/__mocks__/ErrorUtils.js
 Please delete one of the following two files: 
 /home/ubuntu/react-native-pathjs-charts/node_modules/react-native/Libraries/Core/__mocks__/ErrorUtils.js
/home/ubuntu/react-native-pathjs-charts/node_modules/react-native/node_modules/fbjs/lib/__mocks__/ErrorUtils.js

Unfortunately, Jest doesn't really give you an option for avoiding these warnings - see this open issue.

Copy link
Contributor

@katscott katscott left a comment

Choose a reason for hiding this comment

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

couple questions

<View>
<Pie
data={data}
<ScrollView horizontal={true} style={{flex:1,backgroundColor:'#F5FCFF'}} contentContainerStyle={{justifyContent:'center',alignItems:'center'}}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is a ScrollView actually needed here? (i've not opened example app just yet to view this)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not needed and was a bit accidental but I don't see it as harmful. It can actually be useful if someone changes the chart size beyond what can be rendered on one screen. Willing to change it back if you think it makes more sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say it would probably make best sense as a specific scroll demo chart for a column bar chart... pie chart would not be as likely to scroll horizontally. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Completely fair point. I changed it back

@@ -3,12 +3,16 @@
"version": "0.0.1",
"private": true,
"scripts": {
"start": "node node_modules/react-native/local-cli/cli.js start"
"start": "node node_modules/react-native/local-cli/cli.js start",
"sync-rnpc": "rm -rf ./node_modules/react-native-pathjs-charts; sane '/usr/bin/rsync -v -a --exclude .git --exclude example --exclude node_modules ../ ./node_modules/react-native-pathjs-charts/' .. --glob='{**/*.json,**/*.js}'"
Copy link
Contributor

Choose a reason for hiding this comment

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

The documentation for the old way was mostly removed, but doesn't look like readme was updated (remove old info and update with new)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Done

@marzolfb marzolfb merged commit c55c723 into master Jan 6, 2017
@marzolfb marzolfb deleted the pie-multicolor branch January 21, 2017 18:57
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.

2 participants