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

Add call to xAxisPaddingUnit for stack-mixin when calculating max/min #1234

Closed
wants to merge 2 commits into from
Closed

Add call to xAxisPaddingUnit for stack-mixin when calculating max/min #1234

wants to merge 2 commits into from

Conversation

stillesjo
Copy link
Contributor

Hi!

We need to get xAxisPaddingUnit when calculating max/min for stack-mixin in order to get xAxisPadding with other units than day to work.

Extends: ea511e0
Reference: #892

@gordonwoodhull gordonwoodhull added this to the v2.0 milestone Dec 5, 2016
@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Dec 5, 2016

Whoops! Looks like that's the only other use of xAxisPadding.

I didn't realize we weren't testing xAxisPaddingUnit - the tests for the add and subtract utilities are great. Could you add some tests for the charts too, please? At least one stacked and one unstacked test, to hit both code paths.

Hopefully there are already some tests for the bubble chart, box plot, or scatter plot that use a date x axis - I know there are some for the line chart and bar chart.

expect(chart.xAxisMax()).toEqual(expectedEndDate);
});

it ('should render the right ')
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @stillesjo.

Wow, Jasmine just silently hangs when there's an incomplete test like this one - we should fail the build if there are any pending tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry it isn't ready for merging yet!

I didn't realize that the PR would update with the changes I pushed :(. Just wanted to continue working at home. Sorry about that!

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem, I understood that. I'll wait for you to comment before I merge.

Travis went ahead and tested it, and I commented because I was alarmed that our current tools will silently drop tests on the floor. There was another time this happened due to use of an ES6 feature but let doesn't appear to be a problem this time.

I wonder if we need to upgrade our build tools yet again. :-/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that might explain it. Added expect(false).toEqual(true); while testing earlier and all tests passed successfully. Changing to var makes tests fails as they should. I'll try to avoid ES6 syntax in the future 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Heheh. Oh I thought it was this dangling it() with no callback and not the let - or maybe it depends on the environment, or the moon.

Anyway, there should be more than 1000 tests, if that number drops there's probably something wrong. It sucks that we have to second guess the test runner, it's supposed to be the other way around!

gordonwoodhull added a commit that referenced this pull request Jan 5, 2017
thanks @stillesjo! with a month test that's plenty for this code path

for #1234

note these must get broken when #792 is fixed
gordonwoodhull added a commit that referenced this pull request Jan 5, 2017
@gordonwoodhull
Copy link
Contributor

Thanks @stillesjo, I wrapped up the tests and it's in dc.js 2.0!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants