search

kurkle / chartjs-chart-treemap

Chart.js module for creating treemap charts
MIT License
139 stars 34 forks source link

Evaluate to remove borderWidth configured as object #126

Closed stockiNail closed 2 years ago

stockiNail commented 2 years ago

borderWidth option is configurable as number and as object ({top, right, bottom, left}).

The borderWidth as object enables the feature to have different borderWidth for each side of the box but it's adding complexity to manage other border options (like borderRadius) and also in the calculation of the space, both rect element and labels.

Currently the boxes with borderWidth as object are drawn overlapping the rectangles. This is disabling some additional border options for lineDash, joint and cap style.

Maybe we could evaluate if makes sense to leave or remove it. Or to find out a way (maybe having a look to CHART.JS project where the BarElement can manage the borderWidth as object as well. Maybe TreemapElement can extend BarElement.

kurkle commented 2 years ago

With chart.js v2 the Rectangle element was used, but when I added chart.js v3 support, an internal element was added for the rects (probably to be compatible with both, v2 and v3 of chart.js)

Extending BarElement should be a good thing, unless there is some blocker that I just don't remember.

stockiNail commented 2 years ago

Extending BarElement should be a good thing, unless there is some blocker that I just don't remember.

Anyway I can check in details how BarElement is managing the borderWidth as object, manaing the other border* options too. Juts a matter of time. ;)

stockiNail commented 2 years ago

Extending BarElement should be a good thing, unless there is some blocker that I just don't remember.

I had a quick look to BarElement and de facto is doing all checks we are doing for borderWidth!! let me take time to test the extension of BarElement.

stockiNail commented 2 years ago

I did some tests extending BarElement but as you have written, there are some blockers (i.e. bounding area that is thought to sequence of elements). Maybe the best thing (even if I don't like to copy code) is copy the bar element and adapt to treemap needs. I'm continuing testing and investigating...

stockiNail commented 2 years ago

Using the code of BarElement is really better!!! It's simplifying other parts of the code and it's managing borderWidth as object and borderRadius.... Hopefully I'll submit a draft PR soon (just for your first review if it's ok for u going on this way).

stockiNail commented 2 years ago

PR https://github.com/kurkle/chartjs-chart-treemap/pull/131 it's just the first impl to check how it will but sounds really better.