Reduce copy/pasted code between chart types
There's a lot of copy/pasted code across various chart types. For instance, there are only ~30 lines which differ between src/components/charts/line/line.vue and src/components/charts/area/area.vue, whereas these files are nearly 400 lines long. In other words, more than 90% of these files are the same.
Diff between src/components/charts/line/line.vue and src/components/charts/area/area.vue
diff --git a/src/components/charts/area/area.vue b/src/components/charts/line/line.vue
index 98fc06524..080074d91 100644
--- a/src/components/charts/area/area.vue
+++ b/src/components/charts/line/line.vue
@@ -1,12 +1,14 @@
<!-- eslint-disable vue/multi-word-component-names -->
<script>
/**
- * Area charts as of %12.10 support annotations.
+ * Line charts as of %12.10 support annotations.
* Annotations is composed of a dotted line and an arrow
* at the bottom. The dotted line is constructed
- * with markLine and arrows with markPoint.
+ * with markLine and arrows with markPoint. Most of this
+ * logic is in GitLab and should be eventually moved to
+ * GitLab UI https://gitlab.com/gitlab-org/gitlab/-/issues/213390.
*
- * Similar to how custom tooltips are displayed when area chart
+ * Similar to how custom tooltips are displayed when line chart
* is hovered, a tooltip should be displayed when the annotation
* arrow is hovered. This component adds event listeners
* to figure out if mouse is hovered on charts to show tooltips.
@@ -17,13 +19,14 @@
*/
import merge from 'lodash/merge';
+import { gray200 } from '../../../../scss_to_js/scss_variables'; // eslint-disable-line import/no-unresolved
import {
defaultChartOptions,
grid,
getThresholdConfig,
generateAnnotationSeries,
dataZoomAdjustments,
- defaultAreaOpacity,
+ symbolSize,
mergeSeriesToOptions,
mergeAnnotationAxisToOptions,
lineStyle,
@@ -81,11 +84,6 @@ export default {
required: false,
default: true,
},
- formatAnnotationsTooltipText: {
- type: Function,
- required: false,
- default: null,
- },
formatTooltipText: {
type: Function,
required: false,
@@ -119,6 +117,11 @@ export default {
return [LEGEND_LAYOUT_INLINE, LEGEND_LAYOUT_TABLE].indexOf(layout) !== -1;
},
},
+ showLegend: {
+ type: Boolean,
+ required: false,
+ default: true,
+ },
/**
* Sets the chart's height in pixels. Set to `"auto"` to use the height of the container.
*/
@@ -157,15 +160,11 @@ export default {
series() {
const dataSeries = this.data.map((series, index) => {
const defaultColor = colorFromDefaultPalette(index);
- const getColor = (type) =>
- series[type] && series[type].color ? series[type].color : defaultColor;
+ const getColor = (type) => series[type]?.color ?? defaultColor;
+
return merge(
{
- areaStyle: {
- opacity: defaultAreaOpacity,
- color: getColor('areaStyle'),
- },
- showSymbol: false,
+ showSymbol: true,
lineStyle: {
color: getColor('lineStyle'),
},
@@ -173,6 +172,7 @@ export default {
color: getColor('itemStyle'),
},
},
+ symbolSize,
lineStyle,
series,
getThresholdConfig(this.thresholds)
@@ -189,21 +189,20 @@ export default {
return generateAnnotationSeries(this.annotations);
},
options() {
- const defaultAreaChartOptions = {
+ const defaultLineChartOptions = {
xAxis: {
axisPointer: {
show: true,
- lineStyle: {
- type: 'solid',
- },
label: {
formatter: this.onLabelChange,
},
},
- },
- yAxis: {
axisTick: {
- show: false,
+ alignWithLabel: true,
+ show: true,
+ lineStyle: {
+ color: gray200,
+ },
},
},
legend: {
@@ -213,12 +212,12 @@ export default {
const mergedOptions = merge(
{},
defaultChartOptions,
- defaultAreaChartOptions,
+ defaultLineChartOptions,
this.option,
dataZoomAdjustments(this.option.dataZoom)
);
// All chart options can be merged but series
- // needs to be handled specially.
+ // needs to be handled specially
return mergeSeriesToOptions(
mergeAnnotationAxisToOptions(mergedOptions, this.hasAnnotations),
this.series
@@ -243,6 +242,9 @@ export default {
legendStyle() {
return { paddingLeft: `${grid.left}px` };
},
+ hasLegend() {
+ return this.compiledOptions && this.showLegend;
+ },
seriesInfo() {
return this.compiledOptions.series.reduce((acc, series, index) => {
if (series.type === 'line') {
@@ -264,8 +266,8 @@ export default {
this.chart.getDom().removeEventListener('mousemove', this.debouncedShowHideTooltip);
this.chart.getDom().removeEventListener('mouseout', this.debouncedShowHideTooltip);
- this.chart.off('mouseout', this.hideAnnotationsTooltip);
- this.chart.off('mouseover', this.onChartMouseOver);
+ this.chart.off('mouseout', this.onChartDataPointMouseOut);
+ this.chart.off('mouseover', this.onChartDataPointMouseOver);
},
methods: {
defaultFormatTooltipText(params) {
@@ -381,16 +383,16 @@ export default {
>
<template #title>
<slot v-if="formatTooltipText" name="tooltip-title"></slot>
- <template v-else>
+ <div v-else>
{{ dataTooltipTitle }}
<template v-if="options.xAxis.name">({{ options.xAxis.name }})</template>
- </template>
+ </div>
</template>
<slot v-if="formatTooltipText" name="tooltip-content"></slot>
<tooltip-default-format v-else :tooltip-content="dataTooltipContent" />
</chart-tooltip>
<chart-legend
- v-if="compiledOptions"
+ v-if="hasLegend"
:chart="chart"
:style="legendStyle"
:series-info="seriesInfo"
Here's a diff-charts.fish that spits out all the pairwise diffs of the various chart types. Using that, it's clear that several of the chart components share a lot of code. The <template> often differs by just a small amount, usually in the tooltip body.
This duplication can lead to maintenance issues, like bugs that affect one chart type but not another. An example of this is gitlab-org/gitlab-ui!2908 (merged).
Edited by Mark Florian