Using other shapes than "circles" for line points #question

[julien]

Description

This is more a question rather than an issue.

I'd like to be able to use other shapes than svg circle for the "line points" used in Line and Spline charts.

Since circle seems to be hardcoded in many places, I thought using the onafterinit callback to do the following:

• Select chart lines
• Get circle positions (is this possible through the API?)
• Append the "desired shape" (in my case an svg rect) and set it's attributes
• Remove the circles (or hide them via API)

What do you think of the idea?

Would this be an interesting feature for the library?

Thanks

[netil]

@julien that's a good idea!

But rather than applying using callback, it'll be great if this feature can be implemented as one of the new option.

We already have point option, so maybe adding type to change the shape of the data points.

• https://naver.github.io/billboard.js/release/latest/doc/Options.html#.point

point: {
	type: "rect",  // default 'circle'
	...
}

And the expected result will be as:

What do you think on that?

[julien]

@netil sounds like a much better idea indeed.

[julien]

Hi again @netil,

I went ahead an started "prototyping" the type option for point, it probably needs to be much improved, and I'd need some advice and thoughts on the direction to be taken to make this feature as usefull as possible without adding too much complexity to the code (which is kind of what I did in the prototype).

The changes made can be seen in the following diff

I have not yet submitted a pull request, because I think some things are missing:

• The selection also seems to update points (or circles in this case) and would probably need to be updated accordingly.

• No new tests have been added (I thought it might not be that useful for a prototype), but the changes don't break existing tests.

• Since this would allow different types to be used, it might make sense to support other SVG shapes.
  At the moment, code that renders these shapes (or points) is not easily customizable and I
  thought that it might be better to allow the developer to specify this in the options and refactor the existing code for rendering "circles" to follow this pattern.

  Something like.

  point: {
    r: 4,
    render(data, options) {
      return some_d3_selection;
    },
    update(selection, data, options) {
      // update existing shape
      return some_d3_selection;
    }
  }

  However, I'm not really sure how to implement these "factory" methods and would appreciate
  your feedback.

I've also gone an updated the demo in my fork, if you're interested.

While I'm at it, I wanted to tell I'd probably want to collaborate on "custom lines" feature and I hope you don't mind.

Please tell me what you think when you have a chance and thanks for your time

[netil]

That's great @julien and apologize for the delay.

Selection

I can't say the selection circle should follow to the data point shape. Even data point is rectangle, I'm not seeing weird having circle around it.

Tests

Well, the test should be added according the final implementation. So, it should have some test code before the PR.

Types

IMO, the reasonable svg shapes to be support will be:

• circle
• rect
• ellipse (there's no big difference with circle. I don't think is useful)
• polygon (maybe useful)

It'll be great giving some flexibility, but I'm not really sure implementing the factory.
Internally, the rendering of the circle flows from updateCircle() to redrawCircle().

• updateCircle(): Bind the data and add elements with the basic attr.
• redrawCircle(): Positioning elements.

To not break(or follow) the current flow, it's necessary (1) set(updateCircle) and (2) positioning(redrawCircle), but adding 2 callback options isn't seem clear and understandable at first glance.

For this moment, I just come up the following.
Make type to handle node reference also, and draw callback to set points' position and style.

point: {
	type: [HTMLElement | SVGElement],
	draw: function(point, x, y) {
		point.attr("cx", x).attr("cy", y);
	}
}

But, I think is fine just supporting rect type only for now.
Adding more functionality can be done after seeing the reactions of the users.

And it seems great about custom lines. :)

[julien]

Hi @netil,

Thanks for the reply.

I went along and tried using other shapes in my fork just to try and see what I could come up with.

Selection

Concerning the selection, you made a point, I think using a circle even if the shape is a rect shouldn't be an issue (I've tested this with other shapes and it wasn't a problem).

Tests

Concerning the tests, I will add some to make sure everything is covered, I just wanted to have your thoughts after talking about this idea.

Types

Concerning the types, let me clarify what I meant by "factory" method.

As you mentioned, I see that the shape is appended with updateCircle() and that it's posisition is the updated when redrawCircle(...) is invoked.

My proposal would be to do something like this in shape.line.js:

updateCircle() {
  const $$ = this;

  if (!$$.config.point_show) {
    return;
  }

  $$.mainCircle = $$.main.selectAll(`.${CLASS.circles}`).selectAll(`.${CLASS.circle}`)
    .data($$.lineOrScatterData.bind($$));

  $$.mainCircle.exit().remove();

  /* Call the draw method provided via config to append the element */
  const pointType = $$.config.point_type || "circle";
  $$.mainCircle = $$.config.point_create($$.config.point_type)
}

And then to update the circle:

redrawCircle(cx, cy, withTransition, flow) {
	let selectedCircles = this.main.selectAll(`.${CLASS.selectedCircle}`);
	let mainCircles;

	if (!this.config.point_show) {
		return [];
	}

        /* Call the update method provided via config  to update the element's position */
       mainCircles = this.config.point_update(this.mainCircle, cx, cy, withTransition, flow);

	return [
		mainCircles,
		selectedCircles
			.attr("cx", cx)
			.attr("cy", cy)
	];
},

This seems flexible enough to support various shapes, but we would need to check that both methods are provided (create and update) before invoking them. If no methods are provided, we could just use the default "circle" create and update methods (that billboard.js would provide).

• Concerning the lines, I will give you and update shortly but I would like to implement this first.

In anycase, tell me what you think, I will prepare a PR with the code changes (and tests).

Thanks again

[julien]

Hi again @netil,

Since code might be better to explain the "factory methods", I've pushed a preview of these changes here.

With these changes, you could do something like this:

var chart = bb.generate({
  data: {
    columns: [
      ["data1", 30, 200, 100, 400, 150, 250],
      ["data2", 50, 20, 10, 40, 15, 25]
    ]
  },
  point: {
    create: function(parent, cssClassFn, sizeFn, fillStyle) {
      return parent.enter().append("rect")
        .attr("class", cssClassFn)
        .attr("width", sizeFn)
        .attr("height", sizeFn)
        .style("fill", fillStyle);
    },

    update(element, selectedElements, cx, cy, withTransition, flow, opacityFn, fillStyle) {
      let mainCircles;

      const w = parseFloat(element.attr("width"));
      const h = parseFloat(element.attr("height"));

      mainCircles = element
      .attr("x", d => {
        return cx(d) - (w * 0.5);
      })
      .attr("y", d => {
        return cy(d) - (h * 0.5);
      })
      .style("opacity", opacityFn)
      .style("fill", fillStyle);
    
      return mainCircles;
    }
  },
  bindto: "#chart"
});

And the line would use "rect"s instead of "circles".
Tell me if you think this is still a good idea when you can.

Thanks

[netil]

What a great approach @julien!
I love the idea, but also have some concern on this.

Giving flexibility is really good, but the user should have some kind of knowledge to implement this. I mean, this options are more likely for 'high-end' users.

I think billboard.js is getting popularity because of its simplicity in the implementation and ease usage interface.

What do you think about this? If there is more simplicity way to get on this, will be great.

[julien]

Thanks @netil,

I understand you have some concerns about this suggestion, and also agree that in order to implement this feature, the developer should have some idea on how to implement it.
Documenting and providing examples, would make things easier.

I also agree that billboard is popular because of it's simplicity, but I also think that allowing the developer to customize the "look and feel" of the charts is a bonus.

One example of customizing billboard is the possibility to change the "default colors" that are provided by billboard by specifying the color_pattern option.

var chart = bb.generate({
  color: {
    pattern: ['#ff0000', '#00ff00'] // My custom colors
  },
  data: {
    columns: [
      ['data1', 120, 200, 20, 400, 50, 100],
      ['data2', 20, 100, 210, 80, 60, 230]
    ]
  },
  bindto: '#chart'
});

Having the same flexibility for "custom shapes" would be great, so I guess we have several options:

1. Allow developers to provide code that renders their "custom shapes".
   In this case, if no options are specified, billboard provides some defaults to
   make sure things are rendered correctly, on the other hand if those options are provided,
   it would be the developer's responsibility.

   var chart = bb.generate({
     point: {
       create: function(options) {
         // code provided by the developer
       },
       update: function(x, y) {
         // code provided by the developer
       } 
     },
     data: {
       columns: [
         ['data1', 120, 200, 20, 400, 50, 100],
         ['data2', 20, 100, 210, 80, 60, 230]
       ]
     },
     bindto: '#chart'
   });

2. Support a set of custom shapes, for example circle and rectangle and
   render the corresponding shape accordingly.

   var chart = bb.generate({
     point: {
       type: 'rectangle' // If no `type` option is provided, default to circles
     },
     data: {
       columns: [
         ['data1', 120, 200, 20, 400, 50, 100],
         ['data2', 20, 100, 210, 80, 60, 230]
       ]
     },
     bindto: '#chart'
   });

Tell me if you think one of these options is a viable solution and thanks again for your time.

[netil]

My opinion is supporting 2 ways of implementation will be more flexible. Just letting user decide which way to approach.

But on implementing this, it needs decide which options are considered first if both options are declared.

[julien]

@netil, thanks for the feedback.

I think we could "prioritize" which way of drawing things is used when several options are provided.

I'll try to illustrate this with "pseudo code".

// 1 - Check the `point_type` option to see if
//     it has a valid value (e.g. "circle", "rectangle")
if (isValidPointType(this.config.point_type)) {
} 
// 2 - Check that point_create and point_update are functions
else if (hasValidDrawFunctions(this.config)) {
} 
// 3 -  No option was specified, draw the "default" circles
else {
}

I also the number of arguments passed the point_create and point_update functions could be reduced, and I am updating my example, which I'll give you an update on shortly.

[netil]

I agree with your approach @julien.

We discussed quite a while on it, and I think we made some consensus how to implement.
Now is time to implement it!

[julien]

@netil great news!

[julien]

Hi agian @netil,

I just opened #184, so I guess we will continue discussing there.