feat(shape.line): Allow customization of points

[julien]

Issue

#173

Details

• Add point_type, point_create and point_update options

• Add an example in the demo

[coveralls]

Changes Unknown when pulling 0dfb8f0 on julien:custom-points into ** on naver:master**.

[netil]

Wow, what a great feature implementation!
Great job, @julien.

I left some comments, hope not to be complex doing changes. :)

[netil]

How about having below interface?
When set type value as object having create & update, it treated as to create a custom element.

type: "circle", // or rectangle

// to create a custom type, set create & update functions as well
type: {
 	    create(element, cssClassFn, sizeFn, fillStyleFn) {
				...
				// should create node element to be used as data point and must return a d3.selection
				return element;
      	},
      	update(element, xPosFn, yPosFn, opacityStyleFn, fillStyleFn, withTransition, flow, selectedCircles) {
				...
				// adjust the position & styles to the given element and must return a d3.selection
				return element;
      	}
}

BTW, the current explanation is wrong, because to use custom element, the type should be set"custom".

[julien]

Ok thanks, I updated the doc comments.

[netil]

@julien how about the interface that I proposed?
If type is set as an object, it means creating a customized data point.

IMO is more clear than the current, not having declare type to "custom" and not interfering custom options(create and update) being at the same level when type is 'circle' or 'rectangle'.

[julien]

@netil, sorry I hadn't understood that point should be an object, but this does seem like a better idea, I've updated the code accordingly.

[netil]

It's not a big deal, but what about initialize with circle and remove last else block statement?

let createFn = $$.circle.create;

[julien]

Yes this makes sense and simplifies a bit the method, I updated that as well.

[netil]

Array.includes isn't supported in IE. billboard.js has IE9+ support.

• https://developer.mozilla.org/ko/docs/Web/JavaScript/Reference/Global_Objects/Array/includes

Can be transpiled using one of babel plugin

• https://www.npmjs.com/package/babel-plugin-array-includes

but, to not have another dev dependency, it might be better change in more compatible way.

     return /^(circle|rectangle)$/i.test(this.config.point_type);
     // or using Array.indexOf()

[julien]

Perfect thanks, I've updated my PR with this code.

[netil]

It's using config.ponint_r value to determine the size of the data point.
But, when do mouseover(or interaction) on data point, isn't get changed bigger than normal size.

Is because of below code. Is changing r attribute value, which only works for <circle>

billboard.js/src/internals/shape.line.js

Lines 526 to 537 in cf49d71

	expandCircles(i, id, reset) {
	const $$ = this;
	const r = $$.pointExpandedR.bind($$);
	if (reset) {
	$$.unexpandCircles();
	}
	$$.getCircles(i, id)
	.classed(CLASS.EXPANDED, true)
	.attr("r", r);
	},

I know that is not easy to handle that with squares and customs, but I think it should have a compatibility with the others point options.

[julien]

@netil thant's a good point.
That said I have no idea how this could be applied to custom shapes (for example polygon), I did make an update to the current code so that it would only update the r attribute for circle and not other "shapes" while we think about a solution. (Maybe using css transform and scale is an option?

[netil]

That's good idea. I'll dig into transform:scale if this works :)

[netil]

With transform-origin and transform:scale() can make it happen.
But, in the usage of r option, need to be discussed more.

The actual r means radius, so for other types it means r*2.
I think this can cause a confusion and the real meaning also doesn't fits.

The alternative that I though was:

• use r value in different way when isn't circle.
  Instead of radius, it treats as 'scale'
• set another option key like 'size'
  But, still there'll be a confusion between 'r' and 'size'

expandCircles(i, id, reset) {
	...

	if ($$.config.point_type === "circle") {
		circles.attr("r", r);
	} else {
		let scale = r(circles) / $$.config.point_r 

		circles
			.style("transform-origin", "center")
			.style("transform", `scale(${scale})`);
	}
},

unexpandCircles(i) {
	...

	if ($$.config.point_type === "circle") {
		circles.attr("r", r);
	} else {
		circles
			.style("transform-origin", null)
			.style("transform", null);
	}
},

[julien]

@netil from reading the code, and seeing it was rendering svg "circle" elements, it was quite clear to me that r meant radius. I like the approach of using scale for other shapes rather than having a size option, but that's just my opinion ;)

[coveralls]

Changes Unknown when pulling ecde327 on julien:custom-points into ** on naver:master**.

[netil]

How about integrate this as "triangle" type? That will be really useful.

[julien]

I've intergrated this "triangle" type, however I probably need your help with the update methods when withTransition and flow arguments are used.

[netil]

Got it @julien. I think this PR is great, but we need to have a detailed look on backward compatibility with other options without confusion.

[julien]

@netil thanks for the feedback. I also just updated the demo.

[coveralls]

Changes Unknown when pulling 8b710d7 on julien:custom-points into ** on naver:master**.

[coveralls]

Changes Unknown when pulling e923eda on julien:custom-points into ** on naver:master**.

[netil]

I think we're reaching almost the end.
I'll do the rest to merge :)

[julien]

@netil that's good news thanks.

[netil]

Thanks @julien for excellent work!
I just merged the PR, but unfortunately made some updates due to the options compatibility.
Like flow can't be handle correctly for 'triangle' type (due to the coordinate of polygon node can't be set using attributes) and so on.

With this changes can be a start point using customized data points, so I think it can be improved by time and others feedbacks.

Hope this experienced a lot in good way~!
Gracias.

[julien]

Hey @netil no problem!

I saw the changes and it all looks good to me.

I was wondering if you had a release plan for the next updates?

Thanks again

[netil]

well, if you check the milestone, the next coming is planned at Dec, 15th

• https://github.com/naver/billboard.js/milestones

[julien]

I hadn't seen it, thanks