Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix bar lengths in milliseconds from base #4900

Merged
merged 7 commits into from
Jun 5, 2020
Merged

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Jun 4, 2020

@archmoj archmoj added bug something broken status: reviewable labels Jun 4, 2020
@archmoj archmoj added this to the v1.54.2 milestone Jun 4, 2020
@archmoj archmoj requested a review from alexcjohnson June 4, 2020 21:26
src/traces/bar/calc.js Outdated Show resolved Hide resolved

var xEnd = out.style[2];
expect(xEnd).not.toBe(1580670000000);
expect(xEnd).toBe(1580688000000);
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you're testing toBe, what do you gain from testing not.toBe? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to ensure no one change the test.

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

Very nice - I like the approach of handling this in set_convert so it can be extended later to other situations more easily 🎉

💃

@archmoj archmoj merged commit 6c949ef into master Jun 5, 2020
@archmoj archmoj deleted the bar-milliseconds-from-base branch June 5, 2020 03:07
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bar length in milliseconds
2 participants