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

Charts #232

Merged
merged 5 commits into from
May 12, 2016
Merged

Charts #232

merged 5 commits into from
May 12, 2016

Conversation

jondlm
Copy link
Contributor

@jondlm jondlm commented May 11, 2016

PR Checklist

  • Manually tested across supported browsers
    • Chrome
    • Firefox
    • Safari
    • IE11 (Win 7)
    • Edge (Win 10)
  • Unit tests written (common at minimum)
  • PR has one of the semver- labels
  • Two core team engineer approvals
  • One core team UX approval
  • Improved the color palette page
  • Updated contrib docs with new _lucidIsPrivate convention
  • Several new private components that power charts
  • Cleaned up variables.less
  • Updated generic tests to handle function props that don't start with on
  • Added new colors for charts
  • Added new utilities to support charts

- Improved the color palette page
- Updated contrib docs with new _lucidIsPrivate convention
- Several new private components that power charts
- Cleaned up variables.less
- Updated generic tests to handle function props that don't start with `on`
- Added new colors for charts
- Added new utilities to support charts
* Classes are appended to root element along with existing classes using
* the `classnames` library.
*/
className: any,
Copy link
Contributor

Choose a reason for hiding this comment

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

we're pretty inconsistent about this, but shouldn't this be string

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 flip flopped on this one. I feel like any is better since we're running stuff through classnames which could potentially be useful to a consumer.

Copy link
Contributor

Choose a reason for hiding this comment

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

seems like consumer could just run through classnames if they need to generate a string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool beans, I'm fine with that. I'll fix my stuff up.

@jondlm jondlm closed this May 11, 2016
@jondlm jondlm reopened this May 11, 2016
@@ -0,0 +1,107 @@
// Note: these tests are basically pin tests, given that we're rendering svgs,
Copy link
Contributor

Choose a reason for hiding this comment

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

filename

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

src/components/BarChart/BarChar.spec.jsx

Copy link
Contributor

Choose a reason for hiding this comment

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

should be src/components/BarChart/BarChart.spec.jsx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, thank you!

* @return {array[]} - array of arrays, one for each field
*/
export function groupByFields(collection, fields) {
const fieldsArray = [].concat(fields);
Copy link
Contributor

Choose a reason for hiding this comment

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

what's this for

Copy link
Contributor Author

@jondlm jondlm May 11, 2016

Choose a reason for hiding this comment

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

Just some sugar to allow me to pass in either a string or an array of strings. I saw @ogupte doing it and I thought it was cool. And I want to be cool

Copy link
Contributor

Choose a reason for hiding this comment

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

it's a way to coerce any value into an array

// Domain
const range = scale.range();
const sign = orient === 'top' || orient === 'left' ? -1 : 1;
const isH = orient === 'top' || orient === 'bottom';
Copy link
Contributor

Choose a reason for hiding this comment

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

is isH short for isHorizontal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, that's right. I wanted to keep it small since I'm using in lots of small ternaries.

@jondlm jondlm merged commit d697e8b into release/1.0.0 May 12, 2016
@jondlm jondlm deleted the feature/229-charts-squashed branch May 12, 2016 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants