Feature/add configurable graph domain #136
Conversation
Can now specify the minX and maxX values
…able-graph-domain
previously the expectation was that all keys were present if a custom config was provided via the api
…dd-configurable-graph-domain
…able-graph-domain
Some cleanup of chart.js. Removed some if statement that checked for percentage or value - this is now driven from the config. Also added the ability to set the domain of the axis. This allows users to for instance for the percentage graphs to range between 0 and 100
…' of https://github.com/OpenUpSA/wazimap-ng-ui into feature/missing-chart-config-keys-added
…dd-configurable-graph-domain
…able-graph-domain
src/js/profile/indicator.js
Outdated
| } | ||
| const indicators = detail.indicators; | ||
|
|
||
| let c = new Chart(config, this.subindicators, this.groups, indicators, 'Percentage', indicator, title); |
There was a problem hiding this comment.
instead of creating the variable indicators in line 47, you could just use it here
let c = new Chart(config, this.subindicators, this.groups, detail.indicators, 'Percentage', indicator, title);There was a problem hiding this comment.
detail needs to be refactored out of this class entirely
…b.com/OpenUpSA/wazimap-ng-ui into feature/add-configurable-graph-domain
…dash Lodash has a better deep merge function which deals with cases that the existing code didn't.
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
| const indicators = { | ||
| 'Age by race': { | ||
| 'groups': { | ||
| 'gender': { |
There was a problem hiding this comment.
Similar blocks of code found in 2 locations. Consider refactoring.
| 'Female': {'subindicator1': {'count': 10}, 'subindicator2': {'count': 20}}, | ||
| 'Male': {'subindicator1': {'count': 30}, 'subindicator2': {'count': 40}}, | ||
| }, | ||
| 'race': { |
There was a problem hiding this comment.
Similar blocks of code found in 2 locations. Consider refactoring.
| expect(chartData[1].value).toBe(expected.subindicator2.count); | ||
| }) | ||
|
|
||
| test('Handles missing group correctly', () => { |
There was a problem hiding this comment.
Similar blocks of code found in 2 locations. Consider refactoring.
| expect(chartData.length).toBe(0) | ||
| }) | ||
|
|
||
| test('Handles missing subindicator correctly', () => { |
There was a problem hiding this comment.
Similar blocks of code found in 2 locations. Consider refactoring.
|
Deploy preview for wazimap-staging ready! Built with commit c478e8d |
| @@ -1,3 +1,4 @@ | |||
| import {merge} from 'lodash'; | |||
There was a problem hiding this comment.
I wanted to point you to lodash, but I thought you might have considered it already :)
| @@ -156,6 +156,25 @@ export class SubindicatorFilter extends Observable { | |||
| }) | |||
| } | |||
|
|
|||
| getFilteredGroups(groups, selectedGroup, selectedFilter) { | |||
There was a problem hiding this comment.
Identical blocks of code found in 2 locations. Consider refactoring.
Description
Refactored chart.js and enabled more detailed configuration
Related Issue
N/A
How to test it locally
set the chart configuration on a chart to something like this
{
types: {
Value: {formatting: '~s', minX: 0, maxX: 100},
Percentage: {formatting: '.0%'}
}
}
the percentage chart should now range between 0 and 100
Screenshots
Changelog
Added
Updated
Refactoring and added configuration
Removed
Checklist
Pull Request
Commits
Code Quality
Testing
No tests written because of testing complication