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

GH-495 Adds the Scatter Chart #504

Merged
merged 1 commit into from Jun 22, 2015

Conversation

Projects
None yet
3 participants
@anmoljagetia
Contributor

anmoljagetia commented Jun 18, 2015

@@ -0,0 +1,6 @@
.axis path,
.axis line {

This comment has been minimized.

@GordonSmith

GordonSmith Jun 18, 2015

Member

Should be prefixed with the scatter class ID

@anmoljagetia anmoljagetia force-pushed the anmoljagetia:scatterPlot branch from e2cd182 to f36ff9c Jun 18, 2015

XYAxis.call(this);
INDChart.call(this);
}

This comment has been minimized.

@GordonSmith

GordonSmith Jun 18, 2015

Member

Remove blank line

.attr("class", "point")
.attr("r", context.pointSize()/2)
.on("click", function (d2, idx) {
context.click(context.rowToObj(d), context._columns[idx + 1]);

This comment has been minimized.

@mzummo

mzummo Jun 18, 2015

Contributor

idx not defined (its i right now)

This comment has been minimized.

@anmoljagetia

anmoljagetia Jun 18, 2015

Contributor

The idx refers to the function defined on line 46. The d element has the row from the array, and then d2 is individual elements from the array (Found with the help of idx).

This comment has been minimized.

@GordonSmith

GordonSmith Jun 18, 2015

Member

Its defined on line 46

This comment has been minimized.

@mzummo

mzummo Jun 18, 2015

Contributor

ah i missed that, odd it was giving me an error about idx ...

Scatter.prototype.implements(INDChart.prototype);
Scatter.prototype.publish("paletteID", "default", "set", "Palette ID", Scatter.prototype._palette.switch(),{tags:['Basic','Shared']});
Scatter.prototype.publish("dotRepresentation", "cross", "set", "Selects the representation for the data", ["circle", "rectangle", "cross"]);

This comment has been minimized.

@GordonSmith

GordonSmith Jun 18, 2015

Member

rename to pointShape and improve description "Point Shape" or such like.

Scatter.prototype.publish("paletteID", "default", "set", "Palette ID", Scatter.prototype._palette.switch(),{tags:['Basic','Shared']});
Scatter.prototype.publish("dotRepresentation", "cross", "set", "Selects the representation for the data", ["circle", "rectangle", "cross"]);
Scatter.prototype.publish("pointSize", 8, "number", "pointSize");

This comment has been minimized.

@GordonSmith

GordonSmith Jun 18, 2015

Member

Needs a better description @mlzummo

this._palette = this._palette.switch(this.paletteID());
var column = this.svgData.selectAll(".data")
.data(this._data, function (d) { var k = d.filter(function(d, i) {return i > 0;}); return k + Math.random(); })

This comment has been minimized.

@GordonSmith

GordonSmith Jun 18, 2015

Member

The "key" function here is worrying, the "random" will probably force all the points to be removed and re-appended on every update call. Can you delete this and I can show you a better way to handle the "shape" changing issue.

.each(function (d, i) {
var element = d3.select(this);
var point = element.selectAll(".point").data(d.filter(function(d, i) {return i > 0;}));
var point1 = element.selectAll(".point").data(d.filter(function(d, i) {return i > 0;}));

This comment has been minimized.

@GordonSmith

GordonSmith Jun 18, 2015

Member

You will want to give this a different class name otherwise it could interfere with the other one.

This comment has been minimized.

@GordonSmith

GordonSmith Jun 18, 2015

Member

Another approach would have been to add a single point of type "g" and append two lines within that.

var title = point
.enter().append("circle")
.attr("class", "point")
.attr("r", context.pointSize()/2)

This comment has been minimized.

@GordonSmith

GordonSmith Jun 18, 2015

Member

This should be set in the "update" cycle not the "enter()" cycle (and it only appears to be working now due to the random above)

.enter().append("rect")
.attr("class", "point")
.attr("width", context.pointSize())
.attr("height", context.pointSize())

This comment has been minimized.

@GordonSmith

GordonSmith Jun 18, 2015

Member

width + height should be applied at the update cycle.

var title = point
.enter().append("line")
.attr("class", "point")
.attr("stroke-width", 2)

This comment has been minimized.

@GordonSmith

GordonSmith Jun 18, 2015

Member

Can this be moved into the style sheet?

var title1 = point1
.enter().append("line")
.attr("class", "point")
.attr("stroke-width", 2)

This comment has been minimized.

@GordonSmith

GordonSmith Jun 18, 2015

Member

Can this be moved into the style sheet?

This comment has been minimized.

@anmoljagetia

anmoljagetia Jun 18, 2015

Contributor

If I move this to style sheet, I will have to color the stroke with every other shape, and the size estimation we have now, will also change. Also this is only relevant for the cross shape.

point.transition()
.attr("cx", function (d2, idx) { return context.dataScale(d[0]) + context.dataScale.rangeBand()/2 ;})
.attr("cy", function (d2, idx) { return context.valueScale(d2); })
.style("fill", function (d2, idx) { return context._palette(idx + 1); })

This comment has been minimized.

@GordonSmith

GordonSmith Jun 18, 2015

Member

All calls to "_palette" should use the column label and not the idx so that the colours are consistent with other charts displaying the same data...

@anmoljagetia anmoljagetia force-pushed the anmoljagetia:scatterPlot branch from f36ff9c to 2ae7b37 Jun 18, 2015

title
.text(function (d2, idx) { return d[0] + " (" + d2 + ")" + ": " + context._columns[idx + 1]; })
;
point.exit().remove();

This comment has been minimized.

@GordonSmith

GordonSmith Jun 18, 2015

Member

point1 will need a remove also?

This comment has been minimized.

@anmoljagetia

anmoljagetia Jun 18, 2015

Contributor

Added the removal of point1 also.

@anmoljagetia anmoljagetia force-pushed the anmoljagetia:scatterPlot branch 3 times, most recently from ea3ed86 to 1f34d85 Jun 18, 2015

}
var column = this.svgData.selectAll(".data")
.data(this._data, function (d) { var k = d.filter(function(d, i) {return i > 0;}); return k; })

This comment has been minimized.

@GordonSmith

GordonSmith Jun 18, 2015

Member

The whole key function is not needed: , function (d) { var k = d.filter(function(d, i) {return i > 0;}); return k; }

this._palette = this._palette.switch(this.paletteID());
if (this._prevPointShape !== this.pointSize()) {

This comment has been minimized.

@GordonSmith

GordonSmith Jun 18, 2015

Member

Should be pointShape()

if (this._prevPointShape !== this.pointSize()) {
this.svgData.selectAll(".data").remove();
this._prevPointShape = this.pointSize();

This comment has been minimized.

@GordonSmith

GordonSmith Jun 18, 2015

Member

Should be pointShape()

title = point
.enter().append("line")
.attr("class", "point")
.attr("stroke-width", 2)

This comment has been minimized.

@GordonSmith

GordonSmith Jun 18, 2015

Member

Can this be a style?

title1 = point1
.enter().append("line")
.attr("class", "point1")
.attr("stroke-width", 2)

This comment has been minimized.

@GordonSmith

GordonSmith Jun 18, 2015

Member

Can this be a style?

This comment has been minimized.

@anmoljagetia

anmoljagetia Jun 18, 2015

Contributor

Adding this to style will add a stroke on everything else as well, i.e. circles and also the rectangles. In that case, we will have to resize everything, and also handle the strokes on circle and rectangles separately.

This comment has been minimized.

@GordonSmith

GordonSmith Jun 19, 2015

Member

Not if you added it as "line.point, line.point1 {...}"

@anmoljagetia anmoljagetia force-pushed the anmoljagetia:scatterPlot branch from 1f34d85 to 78d0fac Jun 18, 2015

GH-495 Adds the Scatter Chart
Fixed GH-495

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

@anmoljagetia anmoljagetia force-pushed the anmoljagetia:scatterPlot branch from 78d0fac to e6019b7 Jun 19, 2015

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

@GordonSmith GordonSmith merged commit e782582 into hpcc-systems:master Jun 22, 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