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 minValueScale option #54

Merged
merged 1 commit into from
Nov 25, 2014
Merged

Conversation

megawac
Copy link
Contributor

@megawac megawac commented Nov 24, 2014

Obvious compliment to maxValueScale. I was looking for it and was a bit surprised it wasn't about

@megawac megawac force-pushed the minval-scale branch 2 times, most recently from 507721c to 7087198 Compare November 24, 2014 17:41
@drewnoakes
Copy link
Collaborator

Looks good! I'll do some testing asap and merge this in if all works well. Thanks.

@megawac
Copy link
Contributor Author

megawac commented Nov 24, 2014

Actually this doesn't work too hot for negative values in the scale. Give me a sec I'm refactoring this

@megawac
Copy link
Contributor Author

megawac commented Nov 24, 2014

Eh, that will do I guess -- ping @drewnoakes

@drewnoakes
Copy link
Collaborator

Nice spot on the fix. Actually you've identified a bug in the maxValueScale scenario too that manifests when the maximum value is negative. Hope to look at merging this in the next evening or two.

drewnoakes added a commit that referenced this pull request Nov 25, 2014
@drewnoakes drewnoakes merged commit eb11578 into joewalnes:master Nov 25, 2014
drewnoakes added a commit that referenced this pull request Nov 25, 2014
@drewnoakes
Copy link
Collaborator

There is actually one minor glitch with this PR.

In the case of maxValueScale, a value less than 1 (say, 0.8) creates a negative margin, such that the series may spill outside the chart area.

With this minValueScale implementation, a value less than 1 has the same effect as a value that's greater than 1.

Would you have a chance to look at it? If so, I'll hold off on tagging the release.

@megawac
Copy link
Contributor Author

megawac commented Nov 25, 2014

Sure i can fix that tonight but why would someone use a value less than 1
On Nov 25, 2014 5:32 PM, "Drew Noakes" notifications@github.com wrote:

There is actually one minor glitch with this PR.

In the case of maxValueScale, a value less than 1 (say, 0.8) creates a
negative margin, such that the series may spill outside the chart area.

With this minValueScale implementation, a value less than 1 has the same
effect as a value that's greater than 1.

Would you have a chance to look at it? If so, I'll hold off on tagging the
release.


Reply to this email directly or view it on GitHub
#54 (comment).

@drewnoakes
Copy link
Collaborator

Haha, well I thought the same thing about minValueScale but I'm sure you had a good reason to want it :) Perhaps a value < 1 would be useful to crop outliers... not very convincing, but there may be a use case. At any rate, it'd be less potentially surprising if both settings behaved in the same way.

@megawac
Copy link
Contributor Author

megawac commented Nov 25, 2014

Agreed, though we probably should be using the range say (scale * (max - min)) * val - val sound good?

@megawac megawac deleted the minval-scale branch November 25, 2014 23:02
HenryNe pushed a commit to HenryNe/smoothie that referenced this pull request Mar 25, 2017
HenryNe pushed a commit to HenryNe/smoothie that referenced this pull request Mar 25, 2017
HenryNe pushed a commit to HenryNe/smoothie that referenced this pull request Mar 25, 2017
HenryNe pushed a commit to HenryNe/smoothie that referenced this pull request Mar 25, 2017
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.

3 participants