Merge branch '2491-approval-align' into 'master'

MR approval placeholders same size as images

Closes #2491

See merge request !2007

app/assets/javascripts/vue_merge_request_widget/ee/components/approvals/approvals_footer.js

 /* global Flash */
-import pendingAvatarSvg from 'icons/_icon_dotted_circle.svg';
 import LinkToMemberAvatar from '~/vue_shared/components/link_to_member_avatar';
 import eventHub from '../../../event_hub';
 
@@ -38,7 +37,6 @@ export default {
   data() {
     return {
       unapproving: false,
-      pendingAvatarSvg,
     };
   },
   components: {
@@ -68,23 +66,22 @@ export default {
   template: `
     <div v-if="approvedBy.length" class="approved-by-users approvals-footer clearfix mr-info-list">
       <div class="legend"></div>
-      <div>
-        <p class="approvers-prefix">Approved by</p>
+      <div class="approvers-prefix">
+        <p>Approved by</p>
         <div class="approvers-list">
           <span v-for="approver in approvedBy">
             <link-to-member-avatar
+              :avatarSize="20"
               extra-link-class="approver-avatar"
-              :avatar-url="approver.user.avatar_url"
               :display-name="approver.user.name"
               :profile-url="approver.user.web_url"
               :show-tooltip="true" />
           </span>
           <span class="potential-approvers-list" v-for="n in approvalsLeft">
             <link-to-member-avatar
+              :avatarSize="20"
               :clickable="false"
-              :avatar-html="pendingAvatarSvg"
-              :show-tooltip="false"
-              extra-link-class="hide-asset" />
+              :show-tooltip="false" />
           </span>
         </div>
         <span class="unapprove-btn-wrap" v-if="showUnapproveButton">

app/assets/javascripts/vue_shared/components/link_to_member_avatar.js

 // Analogue of link_to_member_avatar in app/helpers/projects_helper.rb
+import pendingAvatarSvg from 'icons/_icon_dotted_circle.svg';
 
 export default {
   props: {
@@ -52,6 +53,7 @@ export default {
   data() {
     return {
       avatarBaseClass: 'avatar avatar-inline',
+      pendingAvatarSvg,
     };
   },
   computed: {
@@ -59,7 +61,7 @@ export default {
       return `s${this.avatarSize}`;
     },
     avatarHtmlClass() {
-      return `${this.avatarSizeClass} ${this.avatarBaseClass}`;
+      return `${this.avatarSizeClass} ${this.avatarBaseClass} avatar-placeholder`;
     },
     tooltipClass() {
       return this.showTooltip ? 'has-tooltip' : '';
@@ -84,21 +86,23 @@ export default {
         :class="linkClass"
         :title="displayName"
         :data-container="tooltipContainerAttr">
+        <img
+          v-if="avatarUrl"
+          :class="avatarClass"
+          :src="avatarUrl"
+          :width="avatarSize"
+          :height="avatarSize"
+          :alt="displayName"/>
         <svg
-          v-if="avatarHtml"
-          v-html="avatarHtml"
+          v-else
+          v-html="pendingAvatarSvg"
           :class="avatarHtmlClass"
           :width="avatarSize"
           :height="avatarSize"
           :alt="displayName">
         </svg>
 
-        <img
-          :class="avatarClass"
-          :src="avatarUrl"
-          :width="avatarSize"
-          :height="avatarSize"
-          :alt="displayName"/>
+
       </a>
     </div>
   `,

app/assets/stylesheets/framework/avatar.scss

@@ -59,6 +59,10 @@
     border: none;
   }
 
+  &.avatar-placeholder {
+    border: none;
+  }
+
   &:not([href]):hover {
     border-color: rgba($avatar-border, .2);
   }

app/assets/stylesheets/pages/merge_requests.scss

@@ -817,32 +817,11 @@
     top: 10px;
   }
 
-  // vertically centers all children
-  > span {
-    align-self: center;
-  }
-
-  .hide-asset {
-    img {
-      display: none;
-    }
-
-    svg {
-      margin-bottom: -7px; // makes up for border removed
-      border: none;
-    }
-  }
-
   .approvers-prefix,
   .approvers-list {
+    display: flex;
+    align-items: center;
     margin-right: 5px;
-    float: left;
-  }
-
-  .approvers-list img {
-    width: 18px;
-    height: 18px;
-    margin-top: 3px;
   }
 
   .unapprove-btn {
@@ -860,18 +839,8 @@
     }
   }
 
-  // styles for approver avatar checkmark
   .approver-avatar {
     position: relative;
-    display: inline-block;
-
-    svg.avatar {
-      position: absolute;
-      top: 12%;
-      right: 4%;
-      height: 45%;
-      width: 45%;
-    }
   }
 }
 
@@ -882,8 +851,9 @@
   }
 
   .avatar {
-    margin-bottom: -2px;
-    margin-right: 3px;
+    margin-bottom: 0;
+    margin-left: 7px;
+    display: block;
   }
 }
 

spec/javascripts/vue_shared/components/link_to_member_avatar_spec.js

@@ -20,6 +20,7 @@ import linkToMemberAvatar from '~/vue_shared/components/link_to_member_avatar';
 
     this.$document = $(document);
   }
+
   describe('Link To Members Components', function () {
     describe('Initialization', function () {
       beforeEach(function () {
@@ -41,19 +42,19 @@ import linkToMemberAvatar from '~/vue_shared/components/link_to_member_avatar';
         expect(this.component.$data).toBeDefined();
       });
 
-      it('should have <a> and <img> children', function () {
+      it('should have <a> and <svg> children', function () {
         const componentLink = this.component.$el.querySelector('a');
-        const componentImg = componentLink.querySelector('img');
+        const componentPlaceholder = componentLink.querySelector('svg');
 
         expect(componentLink).not.toBeNull();
-        expect(componentImg).not.toBeNull();
+        expect(componentPlaceholder).not.toBeNull();
       });
 
       it('should correctly compute computed values', function (done) {
         const correctVals = {
           disabledClass: '',
           avatarSizeClass: 's32',
-          avatarHtmlClass: 's32 avatar avatar-inline',
+          avatarHtmlClass: 's32 avatar avatar-inline avatar-placeholder',
           avatarClass: 'avatar avatar-inline s32 ',
           tooltipClass: 'has-tooltip',
           linkClass: 'author_link has-tooltip  ',