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

Fix y-axis graph padding #882

Merged
merged 6 commits into from
Feb 17, 2017
Merged

Fix y-axis graph padding #882

merged 6 commits into from
Feb 17, 2017

Conversation

jaredscheib
Copy link
Contributor

@jaredscheib jaredscheib commented Feb 15, 2017

  • CHANGELOG.md updated
  • Rebased/mergable
  • Tests pass
  • Sign CLA (if not already signed)

Connect #881

The problem

Graph did not properly display highlighted range when value for "greater than" rule exceeded local data peak value. This bug also showed up in "inside range" and "outside range".

The Solution

  • Fixed the math for adding and subtracting padding.
  • Removed incorrect and arbitrary "top" argument to allow "range" rules to display correct highlighting regardless of which value was greater.

Fixed display for "greater than":
screen shot 2017-02-15 at 1 46 14 am

Copy link
Contributor

@121watts 121watts left a comment

Choose a reason for hiding this comment

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

This look good to me. Also checked out the branch and everything seems to be in order. Only thing missing is some tests. Specifically for the bug that's being fixed.

@jaredscheib
Copy link
Contributor Author

Cool, thanks! Getting to tests now and will come back with something.

@jaredscheib
Copy link
Contributor Author

@121watts I added a couple of preliminary tests for your perusal.

Questions:
Maybe I should be generating random values?
Maybe multiple assertions should be in multiple tests?
Maybe the description is unclear?
Maybe the variable names aren't great?

@121watts
Copy link
Contributor

@jaredscheib Once you update the CHANGELOG this is good to merge!

@jaredscheib
Copy link
Contributor Author

jaredscheib commented Feb 17, 2017

Cool, done!

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