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 ability to select y-axis min and max values #3464

Merged
merged 35 commits into from
May 18, 2015

Conversation

stormpython
Copy link
Contributor

Closes #354.

This pr adds the ability to select the y-axis min and max values.

…nal code around checking that the yAxis min and max values are valid
…s, adding functions for verifying user input, fixing failing tests
@panda01 panda01 self-assigned this Apr 6, 2015
@@ -20,7 +20,10 @@ define(function (require) {
opts = opts || {};

return function (vis) {
var isUserDefinedYAxisMin = (vis._attr.setYExtents.min);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why have the parenthesis here?

@@ -91,6 +109,16 @@ define(function (require) {
return numeral(d).format('0.[0]a');
};

YAxis.prototype.tickFormat = function (domain) {
if (this.isPercentage()) return d3.format('%');
if (domain[1] <= 100 && domain[0] >= -100 && !this.isPercentage()) return d3.format('n');
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps a more descriptive if statement would be better suited here.

@stormpython
Copy link
Contributor Author

@panda01 thanks for the earlier comments. I am putting the review tag on it now, so the rest is ready for review.

Conflicts:
	src/kibana/components/errors.js
	src/kibana/components/vislib/lib/y_axis.js
	src/kibana/plugins/vis_types/vislib/area.js
	src/kibana/plugins/vis_types/vislib/line.js
@stormpython
Copy link
Contributor Author

@panda01 Update to my previous comment. Just resolved some conflicts after merging master. I will probably need to add some more tests to make sure things are sound. But please go ahead and review.

<div ng-show="vis.params.setYExtents.max">
<label>
y-max
<input type="number" step="0.1" ng-model="vis.params.yAxis.max" ng-required="vis.params.setYExtents.max">
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use min here to make sure people can't input numbers < 0.

Just a suggestion since I see this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't actually won't to limit people from having values less than 0. You could have values where the min and the max values for the chart are both below zero.

It should only not be zero when the log scale is selected.

Copy link
Contributor

Choose a reason for hiding this comment

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

Whoops, well ignore that then.

Copy link
Contributor

Choose a reason for hiding this comment

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

Need some class="form-control" action here

@panda01
Copy link
Contributor

panda01 commented Apr 9, 2015

2015-04-09 14_00_50
Perhaps for initial values for the min and max value for the y Axis we could use the current values?
Just to make it so that way the user doesn't have to type it in?

@stormpython
Copy link
Contributor Author

@panda01 yeah, that was what I initially wanted to try, but I couldn't figure out how to get those values using ng-model, and after talking with @spalger, he didn't think that was a good idea. Or maybe I miss understood him.

In any case, I love that idea. If you have sometime to help me figure it out, I'd really appreciate it.

@spalger
Copy link
Contributor

spalger commented Apr 9, 2015

@stormpython @panda01 the vis options and the vislibVis object are very, very separate (intentionally). We might be able to figure out the y-bounds using the searchSource's history, but that won't take into account stacking or scaling, all in all I don't think it's worth it.

@panda01
Copy link
Contributor

panda01 commented Apr 20, 2015

This LGTM. After talking with @spalger, it definitely seems like adding the already calculated min and max, is a hassle.

This PR also calls for one more set of eyes.

@panda01 panda01 assigned w33ble and unassigned panda01 Apr 20, 2015
@w33ble
Copy link
Contributor

w33ble commented Apr 23, 2015

Maybe not a blocker, but it seems odd to show an axis error about the min value when the user hasn't supplied a max value.

screenshot 2015-04-22 17 57 29

@panda01
Copy link
Contributor

panda01 commented May 15, 2015

@rashidkpc I don't believe the issue regarding two clicks to validate the form is related to the pull at all. Here is that same issue when messing with agg.
2015-05-15 11_26_03

@panda01
Copy link
Contributor

panda01 commented May 15, 2015

#3848 Should address the aforementioned issue with needing to validate twice

@rashidkpc
Copy link
Contributor

I removed my comment about double validation, fix the tests and I think this is GTG if the clamping issue is solved.

@stormpython
Copy link
Contributor Author

@rashidkpc the clamping issue should be solved in the last push. Also, in one of the tests, when I change 'function' to Function, the tests break. I confirmed that function is the output of the typeof operator, but not sure why it works in one case and not the other. All of what is mentioned here is in my latests push.

@stormpython
Copy link
Contributor Author

@rashidkpc sorry, misread you note, making the corrections

@stormpython
Copy link
Contributor Author

@rashidkpc on a second look, that doesn't seem to work either. Don't have time to figure out why this morning, so I will revert the changes and look into it later.

@rashidkpc
Copy link
Contributor

Should be .to.be.a(Function) instead of .to.be(Function), note the .a()

@stormpython
Copy link
Contributor Author

@rashidkpc thanks, made the corrections.

@stormpython stormpython assigned rashidkpc and unassigned stormpython May 18, 2015
@rashidkpc
Copy link
Contributor

This LGTM. Asking @panda01 to give it a final once over

@rashidkpc rashidkpc assigned panda01 and unassigned rashidkpc May 18, 2015
@panda01
Copy link
Contributor

panda01 commented May 18, 2015

This looks really good. One thing though, when we check the checkbox to enable manual Y min, max we should disable the "Scale Y-Axis to Data Bounds" checkbox, to reduce confusion.

Besides that, this actually LGTM.

@panda01 panda01 assigned stormpython and unassigned panda01 May 18, 2015
@stormpython stormpython assigned panda01 and unassigned stormpython May 18, 2015
rashidkpc added a commit that referenced this pull request May 18, 2015
Add ability to select y-axis min and max values
@rashidkpc rashidkpc merged commit 0128aa9 into elastic:master May 18, 2015
@stormpython stormpython deleted the fixed_scales branch June 19, 2015 20:01
@julienbeclin
Copy link

Hello !

How can we access to this new version ? Has it been added to 4.1?
Thanks

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.

Fixed axis scales
6 participants