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

Refactors X and Y to Data & Column in XYAxis #462

Merged
merged 1 commit into from Jun 11, 2015

Conversation

Projects
None yet
3 participants
@anmoljagetia
Contributor

anmoljagetia commented Jun 8, 2015

@GordonSmith Please Review.
Refactors XYAxis to have dataAxis and columnAxis instead of xAxis and yAxis.

@@ -63,18 +63,18 @@
var test = element.append("g");
var svgXAxis = test.append("g")
var svgDataAxis = test.append("g")

This comment has been minimized.

@GordonSmith

GordonSmith Jun 8, 2015

Member

This is not a "dataAxis" as it sometimes contains columns. It is an XAxis...

;
var svgYAxis = test.append("g")
var svgColumnAxis = test.append("g")

This comment has been minimized.

@GordonSmith

GordonSmith Jun 8, 2015

Member

Same for this: This is not a "columnAxis" as it sometimes contains columns. It is an YAxis...

@@ -49,10 +49,10 @@
this.svg = element.append("g");
this.svgData = this.svg.append("g");
this.svgXAxis = this.svg.append("g")
this.svgDataAxis = this.svg.append("g")

This comment has been minimized.

@GordonSmith

GordonSmith Jun 8, 2015

Member

This is "XAxis" as it is always horizontal.

.attr("class", this.orientation() === "horizontal" ? "x axis" : "y axis")
;
this.svgYAxis = this.svg.append("g")
this.svgColumnAxis = this.svg.append("g")

This comment has been minimized.

@GordonSmith

GordonSmith Jun 8, 2015

Member

This is This is "YAxis" as it is always vertical.

@GordonSmith

This comment has been minimized.

Member

GordonSmith commented Jun 8, 2015

You need to refactor the "x" and "y" in the CSS also.

Refactors X and Y to Data & Column
Removes the generic X and Y reference, and renames the axis to data and column axis.
All the instances of axis are renamed to dataAxis and columnAxis, but the variables x and y are left as it is, as they are heavily used in calculations in the Line and Column files.

Signed-off-by: Anmol Jagetia <anmoljagetia@gmail.com>

@anmoljagetia anmoljagetia force-pushed the anmoljagetia:refactorXY branch from a377c76 to 8fcb7d6 Jun 9, 2015

@mzummo

This comment has been minimized.

Contributor

mzummo commented Jun 9, 2015

I thought we were keeping X and Y axis?

@GordonSmith

This comment has been minimized.

Member

GordonSmith commented Jun 9, 2015

@mlzummo These charts can now swap orientation (think bar / column) the styles are directed at the "data values" and "columns" rather than the x / y axis, which is why we are renaming some of the variable / class names (Note: there are still some true x / y variables which should remane).

@GordonSmith

This comment has been minimized.

Member

GordonSmith commented Jun 11, 2015

Looks good.

GordonSmith added a commit that referenced this pull request Jun 11, 2015

Merge pull request #462 from anmoljagetia/refactorXY
Refactors X and Y to Data & Column in XYAxis

@GordonSmith GordonSmith merged commit 59f943a into hpcc-systems:master Jun 11, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment