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

Log along the x-axis (bug 189) #296

Merged
merged 4 commits into from May 29, 2014
Merged

Log along the x-axis (bug 189) #296

merged 4 commits into from May 29, 2014

Conversation

@kberg
Copy link
Collaborator

kberg commented Apr 23, 2014

No description provided.

Robert Konigsberg added 2 commits Apr 23, 2014
@kberg kberg changed the title Log along the x-axis Support log scale on the x axis Apr 23, 2014
@kberg kberg changed the title Support log scale on the x axis Log along the x-axis (bug 189) Apr 23, 2014
context.dateRange = xRange[1] - xRange[0];
context.initialLeftmostDate = xRange[0];

if (g.getOptionForAxis("logscale", 'x')) {

This comment has been minimized.

@danvk

danvk Apr 23, 2014 Owner

single quotes? double quotes?

This comment has been minimized.

@kberg

kberg Apr 23, 2014 Author Collaborator

What, you don't appreciate art? Fixed.

@@ -207,6 +212,14 @@ DygraphLayout.prototype._evaluateLimits = function() {
}
};

DygraphLayout.calcXNormal_ = function(value, axis, logscale) {

This comment has been minimized.

@danvk

danvk Apr 23, 2014 Owner

Is DygraphAxisType really DygraphYAxisType? This code is duplicated and I'm not sure that it needs to be.

This comment has been minimized.

@kberg

kberg Apr 24, 2014 Author Collaborator

What are you referring to?

The code needs to be duplicated because of the x-prefixes. Also, I wouldn't stress too much over five duplicate lines, though I agree clean-up would be nice.

@@ -175,6 +175,7 @@ DygraphLayout.prototype.setYAxes = function (yAxes) {
};

DygraphLayout.prototype.evaluate = function() {
this._xAxis = {};

This comment has been minimized.

@danvk

danvk Apr 23, 2014 Owner

Looks like most member vars have trailing underscores. I should really clean this up sometime.

This comment has been minimized.

@kberg

kberg Apr 24, 2014 Author Collaborator

ack. Agreed, and, no-op.

@@ -183,11 +184,15 @@ DygraphLayout.prototype.evaluate = function() {

DygraphLayout.prototype._evaluateLimits = function() {
var xlimits = this.dygraph_.xAxisRange();
this.minxval = xlimits[0];
this.maxxval = xlimits[1];
this._xAxis.minxval = xlimits[0];

This comment has been minimized.

@danvk

danvk Apr 23, 2014 Owner

why do you need to call this "minxval" if it's an x-axis? Why not minval? Would an object ever have a minxval and a minyval?

This comment has been minimized.

@kberg

kberg Apr 24, 2014 Author Collaborator

Because the related yAxes (yAxis?) field has {min,max}yval as its values, so I went for consistency.

This comment has been minimized.

@danvk

danvk May 12, 2014 Owner

I don't see much point in being consistent with a silly precedent. Is it hard to fix this? The underscore indicates that _xAxis is an internal type, so there shouldn't be any downsides to changing its properties.

This comment has been minimized.

@kberg

kberg May 12, 2014 Author Collaborator

Fixed. I renamed axis to xAxis for CalcXNormal to help clarify which axis is expected at that point.

@@ -734,6 +734,14 @@ Dygraph.prototype.optionsViewForAxis_ = function(axis) {
if (axis_opts && axis_opts[axis] && axis_opts[axis].hasOwnProperty(opt)) {
return axis_opts[axis][opt];
}

// I don't like that this is in a second spot.

This comment has been minimized.

@danvk

danvk Apr 23, 2014 Owner

What is "this" here? It's not clear what the other location is that comment is referring to.

This comment has been minimized.

@kberg

kberg Apr 24, 2014 Author Collaborator

I didn't like repeating the special case of computing the logscale attribute for x-axes such that it ignores the global user-specified setting. I also don't like the optionsViewForAxis code either. :) Advice on code / comment?

* (totally old values) and 1.0 (totally new values) for each frame.
* @private
*/
Dygraph.zoomAnimationFunction = function(frame, numFrames) {

This comment has been minimized.

@danvk

danvk Apr 23, 2014 Owner

I'm not sure why this is showing up in the diff -- did you move it?

This comment has been minimized.

@kberg

kberg Apr 24, 2014 Author Collaborator

I did. It was put smack dab in the middle of related methods.

@kberg
Copy link
Collaborator Author

kberg commented May 10, 2014

ping!

kberg added a commit that referenced this pull request May 29, 2014
Log along the x-axis (bug 189)
@kberg kberg merged commit ebf77a9 into master May 29, 2014
@kberg kberg deleted the xlog branch May 29, 2014
@danvk danvk restored the xlog branch Jun 13, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.