fix(1852): fix property issues with interfaces used by demos #1853
Conversation
✅ Deploy Preview for carbon-charts-angular ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-charts-react ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-charts-core ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| @@ -109,6 +109,10 @@ export interface BasedAxisOptions { | |||
| * axis that's being broken into bins | |||
| */ | |||
| binned?: boolean | |||
| /** | |||
There was a problem hiding this comment.
Used by more than just combo charts
| @@ -106,6 +106,9 @@ export interface BaseChartOptions { | |||
| */ | |||
| prefix?: string | |||
| } | |||
| canvasZoom?: { | |||
| /** | ||
| * options for the points | ||
| */ | ||
| points?: { |
There was a problem hiding this comment.
points property used by Lollipop, Combo, and Area chart demos so moved it to AxisChartOptions.
| /** | ||
| * options for the points | ||
| */ | ||
| points?: { |
There was a problem hiding this comment.
While having this on ScatterChartOptions served Lollipops which use the same options, did nothing for Area and Combo charts
| /** | ||
| * options for the curve of the line | ||
| */ | ||
| curve?: string | { name: string } |
There was a problem hiding this comment.
Moved to AxisChartOptions since it's needed for combo and line chart demos as well.
| @@ -435,6 +432,8 @@ export interface MeterChartOptions extends BaseChartOptions { | |||
| proportional?: { | |||
| total?: number | |||
| unit?: string | |||
| totalFormatter?: (total: number) => string | |||
There was a problem hiding this comment.
Meter chart demos use both of these functions yet they were not established properties
| @@ -509,11 +508,11 @@ export interface TreeChartOptions extends BaseChartOptions { | |||
| */ | |||
| export interface CirclePackChartOptions extends BaseChartOptions { | |||
| circlePack?: { | |||
| circles: { | |||
| circles?: { | |||
There was a problem hiding this comment.
Most of the circle pack chart demos did not have this property so making it optional
…not `hierarchyLevel`
| @@ -28,7 +28,7 @@ export class CirclePackChartModel extends ChartModel { | |||
| updateLegendAdditionalItems(options, zoomOptions) | |||
|
|
|||
| const depth = this.getHierarchyLevel() | |||
| const userProvidedDepth = getProperty(options, 'circlePack', 'hierarchyLevel') | |||
There was a problem hiding this comment.
If you looked at functions like getHierarchyLevel(), you'll see that they expected a depth property - not hierarchyLevel.
| @@ -206,7 +210,7 @@ export class Tooltip extends Component { | |||
| return valueFormatter(value, label) | |||
| } | |||
|
|
|||
| if (typeof value.getTime === 'function') { | |||
| if (this.isDate(value)) { | |||
There was a problem hiding this comment.
Cleaner check with TypeScript type predicate
| @@ -44,7 +44,7 @@ export const circlePack = { | |||
| mainGroup: 4, | |||
| children: 2 | |||
| }, | |||
| hierarchyLevel: 2 | |||
There was a problem hiding this comment.
Implementation always expected depth - not hierarchyLevel
| @@ -224,7 +224,7 @@ export const circlePackThreeLevelsMonochromeData = [ | |||
| export const circlePackThreeLevelNoZoomOptions = { | |||
| title: 'Three Levels Hierarchy (No Zoom)', | |||
| circlePack: { | |||
| hierarchyLevel: 3 | |||
| depth: 3 | |||
There was a problem hiding this comment.
Implementation always expected depth - not hierarchyLevel
| /** | ||
| * options for the curve of the line | ||
| */ | ||
| curve?: string | { name: string } |
There was a problem hiding this comment.
curve is not just for AreaChart (used by line and combo, too).
Updates