improve merge request widget status icon UX

x to indicate failure or cannot merge

2 files changed, 84 insertions, 19 deletions

diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_ready_to_merge.js b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_ready_to_merge.js
index ad709da51ee..a0aba7718d4 100644
--- a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_ready_to_merge.js
+++ b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_ready_to_merge.js
@@ -38,24 +38,40 @@ export default {
 
       return this.useCommitMessageWithDescription ? withoutDesc : withDesc;
     },
-    mergeButtonClass() {
-      const defaultClass = 'btn btn-sm btn-success accept-merge-request';
-      const failedClass = `${defaultClass} btn-danger`;
-      const inActionClass = `${defaultClass} btn-info`;
+    status() {
       const { pipeline, isPipelineActive, isPipelineFailed, hasCI, ciStatus } = this.mr;
 
       if (hasCI && !ciStatus) {
-        return failedClass;
+        return 'failed';
       } else if (!pipeline) {
-        return defaultClass;
+        return 'success';
       } else if (isPipelineActive) {
-        return inActionClass;
+        return 'pending';
       } else if (isPipelineFailed) {
+        return 'failed';
+      }
+
+      return 'success';
+    },
+    mergeButtonClass() {
+      const defaultClass = 'btn btn-sm btn-success accept-merge-request';
+      const failedClass = `${defaultClass} btn-danger`;
+      const inActionClass = `${defaultClass} btn-info`;
+
+      if (this.status === 'failed') {
         return failedClass;
+      } else if (this.status === 'pending') {
+        return inActionClass;
       }
 
       return defaultClass;
     },
+    iconClass() {
+      if (this.status === 'failed' || !this.commitMessage.length || !this.isMergeAllowed() || this.mr.preventMerge) {
+        return 'failed';
+      }
+      return 'success';
+    },
     mergeButtonText() {
       if (this.isMergingImmediately) {
         return 'Merge in progress';
@@ -208,7 +224,7 @@ export default {
   },
   template: `
     <div class="mr-widget-body media">
-      <status-icon status="success" />
+      <status-icon :status="iconClass" />
       <div class="media-body">
         <div class="mr-widget-body-controls media space-children">
           <span class="btn-group append-bottom-5">
diff --git a/spec/javascripts/vue_mr_widget/components/states/mr_widget_ready_to_merge_spec.js b/spec/javascripts/vue_mr_widget/components/states/mr_widget_ready_to_merge_spec.js
index 03a52f1f91c..b0511c25929 100644
--- a/spec/javascripts/vue_mr_widget/components/states/mr_widget_ready_to_merge_spec.js
+++ b/spec/javascripts/vue_mr_widget/components/states/mr_widget_ready_to_merge_spec.js
@@ -95,35 +95,84 @@ describe('MRWidgetReadyToMerge', () => {
       });
     });
 
+    describe('status', () => {
+      it('defaults to success', () => {
+        vm.mr.pipeline = true;
+        expect(vm.status).toEqual('success');
+      });
+
+      it('returns failed when MR has CI but also has an unknown status', () => {
+        vm.mr.hasCI = true;
+        expect(vm.status).toEqual('failed');
+      });
+
+      it('returns default when MR has no pipeline', () => {
+        expect(vm.status).toEqual('success');
+      });
+
+      it('returns pending when pipeline is active', () => {
+        vm.mr.pipeline = {};
+        vm.mr.isPipelineActive = true;
+        expect(vm.status).toEqual('pending');
+      });
+
+      it('returns failed when pipeline is failed', () => {
+        vm.mr.pipeline = {};
+        vm.mr.isPipelineFailed = true;
+        expect(vm.status).toEqual('failed');
+      });
+    });
+
     describe('mergeButtonClass', () => {
       const defaultClass = 'btn btn-sm btn-success accept-merge-request';
       const failedClass = `${defaultClass} btn-danger`;
       const inActionClass = `${defaultClass} btn-info`;
 
-      it('should return default class', () => {
+      it('defaults to success class', () => {
+        expect(vm.mergeButtonClass).toEqual(defaultClass);
+      });
+
+      it('returns success class for success status', () => {
         vm.mr.pipeline = true;
         expect(vm.mergeButtonClass).toEqual(defaultClass);
       });
 
-      it('should return failed class when MR has CI but also has an unknown status', () => {
+      it('returns info class for pending status', () => {
+        vm.mr.pipeline = {};
+        vm.mr.isPipelineActive = true;
+        expect(vm.mergeButtonClass).toEqual(inActionClass);
+      });
+
+      it('returns failed class for failed status', () => {
         vm.mr.hasCI = true;
         expect(vm.mergeButtonClass).toEqual(failedClass);
       });
+    });
 
-      it('should return default class when MR has no pipeline', () => {
-        expect(vm.mergeButtonClass).toEqual(defaultClass);
+    describe('status icon', () => {
+      it('defaults to tick icon', () => {
+        expect(vm.iconClass).toEqual('success');
       });
 
-      it('should return in action class when pipeline is active', () => {
+      it('shows tick for success status', () => {
+        vm.mr.pipeline = true;
+        expect(vm.iconClass).toEqual('success');
+      });
+
+      it('shows tick for pending status', () => {
         vm.mr.pipeline = {};
         vm.mr.isPipelineActive = true;
-        expect(vm.mergeButtonClass).toEqual(inActionClass);
+        expect(vm.iconClass).toEqual('success');
       });
 
-      it('should return failed class when pipeline is failed', () => {
-        vm.mr.pipeline = {};
-        vm.mr.isPipelineFailed = true;
-        expect(vm.mergeButtonClass).toEqual(failedClass);
+      it('shows x for failed status', () => {
+        vm.mr.hasCI = true;
+        expect(vm.iconClass).toEqual('failed');
+      });
+
+      it('shows x for merge not allowed', () => {
+        vm.mr.hasCI = true;
+        expect(vm.iconClass).toEqual('failed');
       });
     });
 
@@ -177,7 +226,7 @@ describe('MRWidgetReadyToMerge', () => {
         expect(vm.isMergeButtonDisabled).toBeTruthy();
       });
 
-      it('should return true when there vm instance is making request', () => {
+      it('should return true when the vm instance is making request', () => {
         vm.isMakingRequest = true;
         expect(vm.isMergeButtonDisabled).toBeTruthy();
       });