-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for stacked bar chart #17
Conversation
c8dc50d
to
0b15108
Compare
@@ -28,11 +28,15 @@ class Chart extends View | |||
['rgba(75, 192, 192, 0.2)', 'rgba(75, 192, 192, 1)'], | |||
['rgba(153, 102, 255, 0.2)', 'rgba(153, 102, 255, 1)'], | |||
['rgba(255, 159, 64, 0.2)', 'rgba(255, 159, 64, 1)'], | |||
['rgba(20, 20, 20, 0.2)', 'rgba(20, 20, 20, 1)'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this come from some design guide? can this can abstracted for unlimited colors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current Colour scheme looks nice and can well be used, I would not abstract or generalize it: devs should change if they prefer other colour coding
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems the original colors come from https://www.chartjs.org/docs/3.9.1/#creating-a-chart example /wo any specific design guide/logic
please add a simple demo usage to |
f22275d
to
837c0c5
Compare
837c0c5
to
014bd37
Compare
014bd37
to
9d402e2
Compare
Added LineChart |
|
||
namespace Atk4\Chart; | ||
|
||
class LineChart extends Chart |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This complicates any extension in user projects... Not sure if different chart type that does not need additional modifications needs a custom class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There can be modifications in future is someone will be ready to contribute them :)
I think each chart type should have it's own class even if it is mostly empty. Also maybe some parts from generic Chart class should go to extended chart classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with this for now.
{ | ||
$this->type = 'horizontalBar'; | ||
|
||
// in chartjs 3.9.1 replace with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to upgrade to the latest lib now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do
Adding feature to support stacked bars.
Also additional default colour for longer datasets added.