Highcharts: Stacked bar#10
Conversation
| const chart = this; | ||
|
|
||
| if (chart.svgElements) { | ||
| chart.svgElements.forEach(e => e.destroy()); |
There was a problem hiding this comment.
It's better to update coords instead. Should fix this
AndrzejBuleczka
left a comment
AndrzejBuleczka
left a comment
There was a problem hiding this comment.
Great work, congrats!
You already get the visual result, which is great, so in that respect the exercise looks solved.
Still few things to consider: I see that you are using a lot of abstraction and separation patterns, which are understandable, but in highcharts we have a different pattern - if you can keep the logic inside the relevant chart config functions (eg. chart.events or label.formatter), we prefer that path;
You correctly noted that destroying and recreating labels on every render is not that great idea, there is a way to draw them once, and just update() position as needed;
finally, there is no need to refer to dataTable here ;) it's simpler to just specify xAxis categories as categories array, and give series data as data array, no need for this extra structure.
I added some of the points inline the code.
Looking forward to see your refactor
| const chart = this; | ||
|
|
||
| if (chart.svgElements) { | ||
| chart.svgElements.forEach(e => e.destroy()); |
| dataTable: { | ||
| columns: { | ||
| Categories: ['Data', 'Emails', 'Duplicates', 'Support'], | ||
| Orange: [100, 130, 35, 30], | ||
| Green: [15, 0, 15, 10], | ||
| Blue: [110, 110, 30], | ||
| Red: [15, 0, 10, 5] | ||
| } | ||
| }, |
There was a problem hiding this comment.
Why are you using dataTable here? feels like an overkill
There was a problem hiding this comment.
I don't know. It was just interesting how it works
| const buttonCSS = { 'font-size': '13px' }; | ||
| const labelCSS = { 'font-size': '10px' }; |
There was a problem hiding this comment.
It's better to set such attribs where you create elements with renderer, no need for such an abstraction here
| }, | ||
| chart: { | ||
| type: 'bar', | ||
| events: chartEvents, |
There was a problem hiding this comment.
I see that you like to keep the config small and define all helpers outside, which is a common pattern, but not here - with such small demos we tend to keep all the logic just in the chart config, here under relevant chart events - can you refactor your solution to follow that pattern, please?
| stackLabels: { | ||
| enabled: true, | ||
| formatter: function() { | ||
| return this.total + ' K'; |
There was a problem hiding this comment.
nice, but actually why do we need formatter here? There is a closely related, simpler and more performant way ;)
| }); | ||
|
|
||
| function addLabels(chart) { | ||
| const labelY = -5; |
There was a problem hiding this comment.
you are hardcoding coordinates, and it work, but have you seen this useful api option, which we tend you use for such cases? https://api.highcharts.com/class-reference/Highcharts.Axis#toPixels
| @@ -0,0 +1,92 @@ | |||
| const categories = ['Data', 'Emails', 'Duplicates', 'Support']; | |||
There was a problem hiding this comment.
Comment for the future - this more naturally should just go to the xAxis.categories setup
AndrzejBuleczka
left a comment
AndrzejBuleczka
left a comment
There was a problem hiding this comment.
Nice work, approve from my side ;)
2 participants
No description provided.