From c93cd37767c8384b7f64aa1e39501ec3cacbd0ad Mon Sep 17 00:00:00 2001
From: Jose Ivan Vargas <jvargas@gitlab.com>
Date: Mon, 7 May 2018 21:08:59 +0000
Subject: Resolve "Monitoring graphs - Popover value improvements"

---
 .../javascripts/monitoring/components/graph.vue    |  8 +++--
 .../monitoring/components/graph/flag.vue           | 22 +++++++------
 .../monitoring/components/graph/path.vue           | 22 +++++++++++--
 .../monitoring/components/graph/track_line.vue     | 10 +++---
 .../monitoring/mixins/monitoring_mixins.js         | 20 ++++++++----
 .../monitoring/utils/date_time_formatters.js       |  2 +-
 .../monitoring/utils/multiple_time_series.js       |  1 +
 app/assets/stylesheets/pages/environments.scss     | 37 ++++++++++++++++++++--
 spec/javascripts/monitoring/graph/flag_spec.js     | 19 +++++++----
 .../monitoring/graph/track_line_spec.js            | 10 +++---
 spec/javascripts/monitoring/graph_path_spec.js     |  2 ++
 spec/javascripts/monitoring/graph_spec.js          |  7 ----
 12 files changed, 111 insertions(+), 49 deletions(-)

diff --git a/app/assets/javascripts/monitoring/components/graph.vue b/app/assets/javascripts/monitoring/components/graph.vue
index f93b1da4f58..de6755e0414 100644
--- a/app/assets/javascripts/monitoring/components/graph.vue
+++ b/app/assets/javascripts/monitoring/components/graph.vue
@@ -81,9 +81,8 @@ export default {
         time: new Date(),
         value: 0,
       },
-      currentDataIndex: 0,
       currentXCoordinate: 0,
-      currentFlagPosition: 0,
+      currentCoordinates: [],
       showFlag: false,
       showFlagContent: false,
       timeSeries: [],
@@ -273,6 +272,9 @@ export default {
             :line-style="path.lineStyle"
             :line-color="path.lineColor"
             :area-color="path.areaColor"
+            :current-coordinates="currentCoordinates[index]"
+            :current-time-series-index="index"
+            :show-dot="showFlagContent"
           />
           <graph-deployment
             :deployment-data="reducedDeploymentData"
@@ -298,9 +300,9 @@ export default {
         :show-flag-content="showFlagContent"
         :time-series="timeSeries"
         :unit-of-display="unitOfDisplay"
-        :current-data-index="currentDataIndex"
         :legend-title="legendTitle"
         :deployment-flag-data="deploymentFlagData"
+        :current-coordinates="currentCoordinates"
       />
     </div>
     <graph-legend
diff --git a/app/assets/javascripts/monitoring/components/graph/flag.vue b/app/assets/javascripts/monitoring/components/graph/flag.vue
index b8202e25685..8a771107de8 100644
--- a/app/assets/javascripts/monitoring/components/graph/flag.vue
+++ b/app/assets/javascripts/monitoring/components/graph/flag.vue
@@ -47,14 +47,14 @@ export default {
       type: String,
       required: true,
     },
-    currentDataIndex: {
-      type: Number,
-      required: true,
-    },
     legendTitle: {
       type: String,
       required: true,
     },
+    currentCoordinates: {
+      type: Array,
+      required: true,
+    },
   },
   computed: {
     formatTime() {
@@ -90,10 +90,12 @@ export default {
     },
   },
   methods: {
-    seriesMetricValue(series) {
+    seriesMetricValue(seriesIndex, series) {
+      const indexFromCoordinates = this.currentCoordinates[seriesIndex]
+      ? this.currentCoordinates[seriesIndex].currentDataIndex : 0;
       const index = this.deploymentFlagData
         ? this.deploymentFlagData.seriesIndex
-        : this.currentDataIndex;
+        : indexFromCoordinates;
       const value = series.values[index] && series.values[index].value;
       if (isNaN(value)) {
         return '-';
@@ -128,7 +130,7 @@ export default {
         <h5 v-if="deploymentFlagData">
           Deployed
         </h5>
-        {{ formatDate }} at
+        {{ formatDate }}
         <strong>{{ formatTime }}</strong>
       </div>
       <div
@@ -163,9 +165,11 @@ export default {
             :key="index"
           >
             <track-line :track="series"/>
-            <td>{{ series.track }} {{ seriesMetricLabel(index, series) }}</td>
             <td>
-              <strong>{{ seriesMetricValue(series) }}</strong>
+              {{ series.track }} {{ seriesMetricLabel(index, series) }}
+            </td>
+            <td>
+              <strong>{{ seriesMetricValue(index, series) }}</strong>
             </td>
           </tr>
         </table>
diff --git a/app/assets/javascripts/monitoring/components/graph/path.vue b/app/assets/javascripts/monitoring/components/graph/path.vue
index 881560124a5..52f8aa2ee3f 100644
--- a/app/assets/javascripts/monitoring/components/graph/path.vue
+++ b/app/assets/javascripts/monitoring/components/graph/path.vue
@@ -22,6 +22,15 @@ export default {
       type: String,
       required: true,
     },
+    currentCoordinates: {
+      type: Object,
+      required: false,
+      default: () => ({ currentX: 0, currentY: 0 }),
+    },
+    showDot: {
+      type: Boolean,
+      required: true,
+    },
   },
   computed: {
     strokeDashArray() {
@@ -33,12 +42,20 @@ export default {
 };
 </script>
 <template>
-  <g>
+  <g transform="translate(-5, 20)">
+    <circle
+      class="circle-path"
+      :cx="currentCoordinates.currentX"
+      :cy="currentCoordinates.currentY"
+      :fill="lineColor"
+      :stroke="lineColor"
+      r="3"
+      v-if="showDot"
+    />
     <path
       class="metric-area"
       :d="generatedAreaPath"
       :fill="areaColor"
-      transform="translate(-5, 20)"
     />
     <path
       class="metric-line"
@@ -47,7 +64,6 @@ export default {
       fill="none"
       stroke-width="1"
       :stroke-dasharray="strokeDashArray"
-      transform="translate(-5, 20)"
     />
   </g>
 </template>
diff --git a/app/assets/javascripts/monitoring/components/graph/track_line.vue b/app/assets/javascripts/monitoring/components/graph/track_line.vue
index 79b322e2e42..18be65fd1ef 100644
--- a/app/assets/javascripts/monitoring/components/graph/track_line.vue
+++ b/app/assets/javascripts/monitoring/components/graph/track_line.vue
@@ -19,16 +19,16 @@ export default {
 <template>
   <td>
     <svg
-      width="15"
-      height="6">
+      width="16"
+      height="8">
       <line
         :stroke-dasharray="stylizedLine"
         :stroke="track.lineColor"
         stroke-width="4"
         :x1="0"
-        :x2="15"
-        :y1="2"
-        :y2="2"
+        :x2="16"
+        :y1="4"
+        :y2="4"
       />
     </svg>
   </td>
diff --git a/app/assets/javascripts/monitoring/mixins/monitoring_mixins.js b/app/assets/javascripts/monitoring/mixins/monitoring_mixins.js
index 6cc67ba57ee..4f23814ff3e 100644
--- a/app/assets/javascripts/monitoring/mixins/monitoring_mixins.js
+++ b/app/assets/javascripts/monitoring/mixins/monitoring_mixins.js
@@ -52,14 +52,22 @@ const mixins = {
     positionFlag() {
       const timeSeries = this.timeSeries[0];
       const hoveredDataIndex = bisectDate(timeSeries.values, this.hoverData.hoveredDate, 1);
+
       this.currentData = timeSeries.values[hoveredDataIndex];
-      this.currentDataIndex = hoveredDataIndex;
       this.currentXCoordinate = Math.floor(timeSeries.timeSeriesScaleX(this.currentData.time));
-      if (this.currentXCoordinate > (this.graphWidth - 200)) {
-        this.currentFlagPosition = this.currentXCoordinate - 103;
-      } else {
-        this.currentFlagPosition = this.currentXCoordinate;
-      }
+
+      this.currentCoordinates = this.timeSeries.map((series) => {
+        const currentDataIndex = bisectDate(series.values, this.hoverData.hoveredDate, 1);
+        const currentData = series.values[currentDataIndex];
+        const currentX = Math.floor(series.timeSeriesScaleX(currentData.time));
+        const currentY = Math.floor(series.timeSeriesScaleY(currentData.value));
+
+        return {
+          currentX,
+          currentY,
+          currentDataIndex,
+        };
+      });
 
       if (this.hoverData.currentDeployXPos) {
         this.showFlag = false;
diff --git a/app/assets/javascripts/monitoring/utils/date_time_formatters.js b/app/assets/javascripts/monitoring/utils/date_time_formatters.js
index f3c9acdd93e..d88c13609dc 100644
--- a/app/assets/javascripts/monitoring/utils/date_time_formatters.js
+++ b/app/assets/javascripts/monitoring/utils/date_time_formatters.js
@@ -14,7 +14,7 @@ const d3 = {
   timeYear,
 };
 
-export const dateFormat = d3.time('%a, %b %-d');
+export const dateFormat = d3.time('%d %b %Y, ');
 export const timeFormat = d3.time('%-I:%M%p');
 export const dateFormatWithName = d3.time('%a, %b %-d');
 export const bisectDate = d3.bisector(d => d.time).left;
diff --git a/app/assets/javascripts/monitoring/utils/multiple_time_series.js b/app/assets/javascripts/monitoring/utils/multiple_time_series.js
index 8a93c7e6bae..4d3f1f1a7cc 100644
--- a/app/assets/javascripts/monitoring/utils/multiple_time_series.js
+++ b/app/assets/javascripts/monitoring/utils/multiple_time_series.js
@@ -123,6 +123,7 @@ function queryTimeSeries(query, graphWidth, graphHeight, graphHeightOffset, xDom
       linePath: lineFunction(timeSeries.values),
       areaPath: areaFunction(timeSeries.values),
       timeSeriesScaleX,
+      timeSeriesScaleY,
       values: timeSeries.values,
       max: maximumValue,
       average: accum / timeSeries.values.length,
diff --git a/app/assets/stylesheets/pages/environments.scss b/app/assets/stylesheets/pages/environments.scss
index 3a300086fa3..1f406cc1c2d 100644
--- a/app/assets/stylesheets/pages/environments.scss
+++ b/app/assets/stylesheets/pages/environments.scss
@@ -283,28 +283,59 @@
   }
 
   &.popover {
+    padding: 0;
+    border: 1px solid $border-color;
+
     &.left {
       left: auto;
       right: 0;
       margin-right: 10px;
+
+      > .arrow {
+        right: -16px;
+        border-left-color: $border-color;
+      }
+
+      > .arrow::after {
+        border-left-color: $theme-gray-50;
+      }
     }
 
     &.right {
       left: 0;
       right: auto;
       margin-left: 10px;
+
+      > .arrow {
+        left: -16px;
+        border-right-color: $border-color;
+      }
+
+      > .arrow::after {
+        border-right-color: $theme-gray-50;
+      }
     }
 
     > .arrow {
-      top: 40px;
+      top: 16px;
+      margin-top: -8px;
+      border-width: 8px;
     }
 
     > .popover-title,
     > .popover-content {
-      padding: 5px 8px;
+      padding: 8px;
       font-size: 12px;
       white-space: nowrap;
     }
+
+    > .popover-title {
+      background-color: $theme-gray-50;
+    }
+  }
+
+  strong {
+    font-weight: 600;
   }
 }
 
@@ -317,7 +348,7 @@
     vertical-align: middle;
 
     + td {
-      padding-left: 5px;
+      padding-left: 8px;
       vertical-align: top;
     }
   }
diff --git a/spec/javascripts/monitoring/graph/flag_spec.js b/spec/javascripts/monitoring/graph/flag_spec.js
index 2d474e9092f..19278312b6d 100644
--- a/spec/javascripts/monitoring/graph/flag_spec.js
+++ b/spec/javascripts/monitoring/graph/flag_spec.js
@@ -22,15 +22,20 @@ const defaultValuesComponent = {
   graphHeightOffset: 120,
   showFlagContent: true,
   realPixelRatio: 1,
-  timeSeries: [{
-    values: [{
-      time: new Date('2017-06-04T18:17:33.501Z'),
-      value: '1.49609375',
-    }],
-  }],
+  timeSeries: [
+    {
+      values: [
+        {
+          time: new Date('2017-06-04T18:17:33.501Z'),
+          value: '1.49609375',
+        },
+      ],
+    },
+  ],
   unitOfDisplay: 'ms',
   currentDataIndex: 0,
   legendTitle: 'Average',
+  currentCoordinates: [],
 };
 
 const deploymentFlagData = {
@@ -113,7 +118,7 @@ describe('GraphFlag', () => {
     });
 
     it('formatDate', () => {
-      expect(component.formatDate).toEqual('Sun, Jun 4');
+      expect(component.formatDate).toEqual('04 Jun 2017, ');
     });
 
     it('cursorStyle', () => {
diff --git a/spec/javascripts/monitoring/graph/track_line_spec.js b/spec/javascripts/monitoring/graph/track_line_spec.js
index 45106830a67..27602a861eb 100644
--- a/spec/javascripts/monitoring/graph/track_line_spec.js
+++ b/spec/javascripts/monitoring/graph/track_line_spec.js
@@ -39,14 +39,14 @@ describe('TrackLine component', () => {
       const svgEl = vm.$el.querySelector('svg');
       const lineEl = vm.$el.querySelector('svg line');
 
-      expect(svgEl.getAttribute('width')).toEqual('15');
-      expect(svgEl.getAttribute('height')).toEqual('6');
+      expect(svgEl.getAttribute('width')).toEqual('16');
+      expect(svgEl.getAttribute('height')).toEqual('8');
 
       expect(lineEl.getAttribute('stroke-width')).toEqual('4');
       expect(lineEl.getAttribute('x1')).toEqual('0');
-      expect(lineEl.getAttribute('x2')).toEqual('15');
-      expect(lineEl.getAttribute('y1')).toEqual('2');
-      expect(lineEl.getAttribute('y2')).toEqual('2');
+      expect(lineEl.getAttribute('x2')).toEqual('16');
+      expect(lineEl.getAttribute('y1')).toEqual('4');
+      expect(lineEl.getAttribute('y2')).toEqual('4');
     });
   });
 });
diff --git a/spec/javascripts/monitoring/graph_path_spec.js b/spec/javascripts/monitoring/graph_path_spec.js
index c83bd19345f..2515e2ad897 100644
--- a/spec/javascripts/monitoring/graph_path_spec.js
+++ b/spec/javascripts/monitoring/graph_path_spec.js
@@ -23,6 +23,7 @@ describe('Monitoring Paths', () => {
       generatedAreaPath: firstTimeSeries.areaPath,
       lineColor: firstTimeSeries.lineColor,
       areaColor: firstTimeSeries.areaColor,
+      showDot: false,
     });
     const metricArea = component.$el.querySelector('.metric-area');
     const metricLine = component.$el.querySelector('.metric-line');
@@ -40,6 +41,7 @@ describe('Monitoring Paths', () => {
         generatedAreaPath: firstTimeSeries.areaPath,
         lineColor: firstTimeSeries.lineColor,
         areaColor: firstTimeSeries.areaColor,
+        showDot: false,
       });
 
       component.lineStyle = 'dashed';
diff --git a/spec/javascripts/monitoring/graph_spec.js b/spec/javascripts/monitoring/graph_spec.js
index 1213c80ba3a..220228e5c08 100644
--- a/spec/javascripts/monitoring/graph_spec.js
+++ b/spec/javascripts/monitoring/graph_spec.js
@@ -30,7 +30,6 @@ describe('Graph', () => {
   it('has a title', () => {
     const component = createComponent({
       graphData: convertedMetrics[1],
-      classType: 'col-md-6',
       updateAspectRatio: false,
       deploymentData,
       tagsPath,
@@ -46,7 +45,6 @@ describe('Graph', () => {
     it('axisTransform translates an element Y position depending of its height', () => {
       const component = createComponent({
         graphData: convertedMetrics[1],
-        classType: 'col-md-6',
         updateAspectRatio: false,
         deploymentData,
         tagsPath,
@@ -62,7 +60,6 @@ describe('Graph', () => {
     it('outerViewBox gets a width and height property based on the DOM size of the element', () => {
       const component = createComponent({
         graphData: convertedMetrics[1],
-        classType: 'col-md-6',
         updateAspectRatio: false,
         deploymentData,
         tagsPath,
@@ -79,7 +76,6 @@ describe('Graph', () => {
   it('sends an event to the eventhub when it has finished resizing', done => {
     const component = createComponent({
       graphData: convertedMetrics[1],
-      classType: 'col-md-6',
       updateAspectRatio: false,
       deploymentData,
       tagsPath,
@@ -97,7 +93,6 @@ describe('Graph', () => {
   it('has a title for the y-axis and the chart legend that comes from the backend', () => {
     const component = createComponent({
       graphData: convertedMetrics[1],
-      classType: 'col-md-6',
       updateAspectRatio: false,
       deploymentData,
       tagsPath,
@@ -111,7 +106,6 @@ describe('Graph', () => {
   it('sets the currentData object based on the hovered data index', () => {
     const component = createComponent({
       graphData: convertedMetrics[1],
-      classType: 'col-md-6',
       updateAspectRatio: false,
       deploymentData,
       graphIdentifier: 0,
@@ -125,6 +119,5 @@ describe('Graph', () => {
 
     component.positionFlag();
     expect(component.currentData).toBe(component.timeSeries[0].values[10]);
-    expect(component.currentDataIndex).toEqual(10);
   });
 });
-- 
cgit v1.2.3