Charts component - developer review?

[manovotn]

Hello

I was assigned a task to cover the brand new charts component with some JS tests.

As I was struggling with the API I realized it might be nice to make a developer review of the code.

So here is what I found troublesome about charts:

1) Examples

If I am about to write a test I need either documentation or some nice examples so I could somehow understand how to create a component and use it. As there is no full documentation I was scavenging for some examples - those are a part of the GitHub repo.

What I found was in no way simple in comparison with, let's say, autocomplete(here) examples. There is a lot of commented out code, also the script parts could be written next to the component they belong to in order to increase readability. Or the scripts could be at least commented as to explain to which chart belongs which script.

Some refactoring would help the cause I believe.

2) Charts component initialization

As you attempt to initialize a chart, there is a need to use something like this (taken from pie chart initialization example):

                               handlers: {"eventFunction":function(event,eventType,seriesIndex,pointIndex,x,y){}} ,

                                particularSeriesHandlers: {"onplotclick":[null],"onplothover":[null]},

                                data: [[{"data":12500746,"label":"Service sector"},{"data":188217,"label":"Agricultural sector"},{"data":2995787,"label":"Industrial sector"}]],   //....

Why do I need to set a handler if it is an empty function? The same goes for second attribute, particularSeriesHandlers, why do I need to set them to null?

If its convinient to use these attributes sometimes, it may be better to make these null values default and the whole attribute then optional. In order to create some basic (eg. default) chart it should be enough to specify it's type (pie/line/...) and it's data (and whatever else cannot be defaultly empty or null).

*) Suggestion

This is merely a suggestion. Maybe we could think of some unified way for future JS components to make examples for them or create documentation (if examples were not enough). So that it would be easier to understand the component's API and then use it (eg. write tests).

[lfryc]

Hey Matej,

thanks for the review.

1) I agree that Chart widget need to be Dev-reviewed and polished first before writing any JavaScript tests.

2) I found this Chart component API rather confusing as well, I will investigate on it

I have created this issue to review: https://github.com/richwidgets/richwidgets/issues/27

3) and that's actually first time I identified the need of some kind of documentation in place, because it's not really much clear what to do, so

I have followed-up with new focused discussion here: Documentation for RichWidgets

I hope Lukas would not mind to review your findings.

[bleathem]

The component documentation provided in the pull reqeust should be kept in mind while we review the richwidget implementation as some of the widget usage may be clarified in the component docs.:

https://github.com/richfaces/richfaces-docs/pull/5

[lukindo]

Hi Matej,

I see your point, the existing documentation and blogs for the chart component targets mainly the JSF use case, JavaScript part is missing.

I'll refactor examples and I'll add comments/docs how to use charts as JavaScript widget.

[lfryc]

Thanks Lukas, it would be awesome!

[manovotn]

Thanks! :-)

[bleathem]

lukindo thanks!

manovotn let's proceed with QE of the component, and comeback to the js API once it's been reviewed, improved and documented.  The facelet API is much more mature than the js API, and should be agnostic to changes introduced to the js API.