Merge branch 'tz-frontend-avatar-resizing' into 'master'

Send resize parameters for avatars

See merge request gitlab-org/gitlab-ce!20966

app/assets/javascripts/lazy_loader.js

 import _ from 'underscore';
 
-export const placeholderImage = 'data:image/gif;base64,R0lGODlhAQABAAAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw==';
+export const placeholderImage =
+  'data:image/gif;base64,R0lGODlhAQABAAAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw==';
 const SCROLL_THRESHOLD = 300;
 
 export default class LazyLoader {
@@ -48,7 +49,7 @@ export default class LazyLoader {
     const visHeight = scrollTop + window.innerHeight + SCROLL_THRESHOLD;
 
     // Loading Images which are in the current viewport or close to them
-    this.lazyImages = this.lazyImages.filter((selectedImage) => {
+    this.lazyImages = this.lazyImages.filter(selectedImage => {
       if (selectedImage.getAttribute('data-src')) {
         const imgBoundRect = selectedImage.getBoundingClientRect();
         const imgTop = scrollTop + imgBoundRect.top;
@@ -66,7 +67,18 @@ export default class LazyLoader {
   }
   static loadImage(img) {
     if (img.getAttribute('data-src')) {
-      img.setAttribute('src', img.getAttribute('data-src'));
+      let imgUrl = img.getAttribute('data-src');
+      // Only adding width + height for avatars for now
+      if (imgUrl.indexOf('/avatar/') > -1 && imgUrl.indexOf('?') === -1) {
+        let targetWidth = null;
+        if (img.getAttribute('width')) {
+          targetWidth = img.getAttribute('width');
+        } else {
+          targetWidth = img.width;
+        }
+        if (targetWidth) imgUrl += `?width=${targetWidth}`;
+      }
+      img.setAttribute('src', imgUrl);
       img.removeAttribute('data-src');
       img.classList.remove('lazy');
       img.classList.add('js-lazy-loaded');

app/assets/javascripts/vue_shared/components/user_avatar/user_avatar_image.vue

 <script>
-
 /* This is a re-usable vue component for rendering a user avatar that
   does not need to link to the user's profile. The image and an optional
   tooltip can be configured by props passed to this component.
@@ -67,7 +66,9 @@ export default {
     // we provide an empty string when we use it inside user avatar link.
     // In both cases we should render the defaultAvatarUrl
     sanitizedSource() {
-      return this.imgSrc === '' || this.imgSrc === null ? defaultAvatarUrl : this.imgSrc;
+      let baseSrc = this.imgSrc === '' || this.imgSrc === null ? defaultAvatarUrl : this.imgSrc;
+      if (baseSrc.indexOf('?') === -1) baseSrc += `?width=${this.size}`;
+      return baseSrc;
     },
     resultantSrcAttribute() {
       return this.lazy ? placeholderImage : this.sanitizedSource;

app/models/concerns/avatarable.rb

@@ -19,7 +19,7 @@ module Avatarable
       # We use avatar_path instead of overriding avatar_url because of carrierwave.
       # See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11001/diffs#note_28659864
 
-      avatar_path(only_path: args.fetch(:only_path, true)) || super
+      avatar_path(only_path: args.fetch(:only_path, true), size: args[:size]) || super
     end
 
     def retrieve_upload(identifier, paths)
@@ -40,12 +40,13 @@ module Avatarable
     end
   end
 
-  def avatar_path(only_path: true)
+  def avatar_path(only_path: true, size: nil)
     return unless self[:avatar].present?
 
     asset_host = ActionController::Base.asset_host
     use_asset_host = asset_host.present?
     use_authentication = respond_to?(:public?) && !public?
+    query_params = size&.nonzero? ? "?width=#{size}" : ""
 
     # Avatars for private and internal groups and projects require authentication to be viewed,
     # which means they can only be served by Rails, on the regular GitLab host.
@@ -64,7 +65,7 @@ module Avatarable
       url_base << gitlab_config.relative_url_root
     end
 
-    url_base + avatar.local_url
+    url_base + avatar.local_url + query_params
   end
 
   # Path that is persisted in the tracking Upload model. Used to fetch the

app/views/admin/projects/_projects.html.haml

@@ -20,7 +20,7 @@
             = link_to(admin_namespace_project_path(project.namespace, project)) do
               .dash-project-avatar
                 .avatar-container.s40
-                  = project_icon(project, alt: '', class: 'avatar project-avatar s40')
+                  = project_icon(project, alt: '', class: 'avatar project-avatar s40', width: 40, height: 40)
               %span.project-full-name
                 %span.namespace-name
                   - if project.namespace

app/views/layouts/nav/sidebar/_project.html.haml

@@ -4,7 +4,7 @@
     .context-header
       = link_to project_path(@project), title: @project.name do
         .avatar-container.s40.project-avatar
-          = project_icon(@project, alt: @project.name, class: 'avatar s40 avatar-tile')
+          = project_icon(@project, alt: @project.name, class: 'avatar s40 avatar-tile', width: 40, height: 40)
         .sidebar-context-title
           = @project.name
     %ul.sidebar-top-level-items

app/views/projects/_home_panel.html.haml

@@ -3,7 +3,7 @@
 .project-home-panel.text-center{ class: ("empty-project" if empty_repo) }
   .limit-container-width{ class: container_class }
     .avatar-container.s70.project-avatar
-      = project_icon(@project, alt: @project.name, class: 'avatar s70 avatar-tile')
+      = project_icon(@project, alt: @project.name, class: 'avatar s70 avatar-tile', width: 70, height: 70)
     %h1.project-title.qa-project-name
       = @project.name
       %span.visibility-icon.has-tooltip{ data: { container: 'body' }, title: visibility_icon_description(@project) }

app/views/projects/edit.html.haml

@@ -51,7 +51,7 @@
           .form-group
             - if @project.avatar?
               .avatar-container.s160.append-bottom-15
-                = project_icon(@project.full_path, alt: '', class: 'avatar project-avatar s160')
+                = project_icon(@project.full_path, alt: '', class: 'avatar project-avatar s160', width: 160, height: 160)
             - if @project.avatar_in_git
               %p.light
                 = _("Project avatar in repository: %{link}").html_safe % { link: @project.avatar_in_git }

app/views/shared/projects/_project.html.haml

@@ -19,7 +19,7 @@
           - if project.creator && use_creator_avatar
             = image_tag avatar_icon_for_user(project.creator, 40), class: "avatar s40", alt:''
           - else
-            = project_icon(project, alt: '', class: 'avatar project-avatar s40')
+            = project_icon(project, alt: '', class: 'avatar project-avatar s40', width: 40, height: 40)
     .project-details
       %h3.prepend-top-0.append-bottom-0
         = link_to project_path(project), class: 'text-plain' do

spec/features/uploads/user_uploads_avatar_to_profile_spec.rb

@@ -15,7 +15,7 @@ describe 'User uploads avatar to profile' do
 
     visit user_path(user)
 
-    expect(page).to have_selector(%Q(img[data-src$="/uploads/-/system/user/avatar/#{user.id}/dk.png"]))
+    expect(page).to have_selector(%Q(img[data-src$="/uploads/-/system/user/avatar/#{user.id}/dk.png?width=90"]))
 
     # Cheating here to verify something that isn't user-facing, but is important
     expect(user.reload.avatar.file).to exist

spec/javascripts/boards/issue_card_spec.js

@@ -69,109 +69,100 @@ describe('Issue card component', () => {
   });
 
   it('renders issue title', () => {
-    expect(
-      component.$el.querySelector('.board-card-title').textContent,
-    ).toContain(issue.title);
+    expect(component.$el.querySelector('.board-card-title').textContent).toContain(issue.title);
   });
 
   it('includes issue base in link', () => {
-    expect(
-      component.$el.querySelector('.board-card-title a').getAttribute('href'),
-    ).toContain('/test');
+    expect(component.$el.querySelector('.board-card-title a').getAttribute('href')).toContain(
+      '/test',
+    );
   });
 
   it('includes issue title on link', () => {
-    expect(
-      component.$el.querySelector('.board-card-title a').getAttribute('title'),
-    ).toBe(issue.title);
+    expect(component.$el.querySelector('.board-card-title a').getAttribute('title')).toBe(
+      issue.title,
+    );
   });
 
   it('does not render confidential icon', () => {
-    expect(
-      component.$el.querySelector('.fa-eye-flash'),
-    ).toBeNull();
+    expect(component.$el.querySelector('.fa-eye-flash')).toBeNull();
   });
 
-  it('renders confidential icon', (done) => {
+  it('renders confidential icon', done => {
     component.issue.confidential = true;
 
     Vue.nextTick(() => {
-      expect(
-        component.$el.querySelector('.confidential-icon'),
-      ).not.toBeNull();
+      expect(component.$el.querySelector('.confidential-icon')).not.toBeNull();
       done();
     });
   });
 
   it('renders issue ID with #', () => {
-    expect(
-      component.$el.querySelector('.board-card-number').textContent,
-    ).toContain(`#${issue.id}`);
+    expect(component.$el.querySelector('.board-card-number').textContent).toContain(`#${issue.id}`);
   });
 
   describe('assignee', () => {
     it('does not render assignee', () => {
-      expect(
-        component.$el.querySelector('.board-card-assignee .avatar'),
-      ).toBeNull();
+      expect(component.$el.querySelector('.board-card-assignee .avatar')).toBeNull();
     });
 
     describe('exists', () => {
-      beforeEach((done) => {
+      beforeEach(done => {
         component.issue.assignees = [user];
 
         Vue.nextTick(() => done());
       });
 
       it('renders assignee', () => {
-        expect(
-          component.$el.querySelector('.board-card-assignee .avatar'),
-        ).not.toBeNull();
+        expect(component.$el.querySelector('.board-card-assignee .avatar')).not.toBeNull();
       });
 
       it('sets title', () => {
         expect(
-          component.$el.querySelector('.board-card-assignee img').getAttribute('data-original-title'),
+          component.$el
+            .querySelector('.board-card-assignee img')
+            .getAttribute('data-original-title'),
         ).toContain(`Assigned to ${user.name}`);
       });
 
       it('sets users path', () => {
-        expect(
-          component.$el.querySelector('.board-card-assignee a').getAttribute('href'),
-        ).toBe('/test');
+        expect(component.$el.querySelector('.board-card-assignee a').getAttribute('href')).toBe(
+          '/test',
+        );
       });
 
       it('renders avatar', () => {
-        expect(
-          component.$el.querySelector('.board-card-assignee img'),
-        ).not.toBeNull();
+        expect(component.$el.querySelector('.board-card-assignee img')).not.toBeNull();
       });
     });
 
     describe('assignee default avatar', () => {
-      beforeEach((done) => {
-        component.issue.assignees = [new ListAssignee({
+      beforeEach(done => {
+        component.issue.assignees = [
+          new ListAssignee(
+            {
               id: 1,
               name: 'testing 123',
               username: 'test',
-        }, 'default_avatar')];
+            },
+            'default_avatar',
+          ),
+        ];
 
         Vue.nextTick(done);
       });
 
       it('displays defaults avatar if users avatar is null', () => {
-        expect(
-          component.$el.querySelector('.board-card-assignee img'),
-        ).not.toBeNull();
-        expect(
-          component.$el.querySelector('.board-card-assignee img').getAttribute('src'),
-        ).toBe('default_avatar');
+        expect(component.$el.querySelector('.board-card-assignee img')).not.toBeNull();
+        expect(component.$el.querySelector('.board-card-assignee img').getAttribute('src')).toBe(
+          'default_avatar?width=20',
+        );
       });
     });
   });
 
   describe('multiple assignees', () => {
-    beforeEach((done) => {
+    beforeEach(done => {
       component.issue.assignees = [
         user,
         new ListAssignee({
@@ -191,7 +182,8 @@ describe('Issue card component', () => {
           name: 'user4',
           username: 'user4',
           avatar: 'test_image',
-        })];
+        }),
+      ];
 
       Vue.nextTick(() => done());
     });
@@ -201,26 +193,30 @@ describe('Issue card component', () => {
     });
 
     describe('more than four assignees', () => {
-      beforeEach((done) => {
-        component.issue.assignees.push(new ListAssignee({
+      beforeEach(done => {
+        component.issue.assignees.push(
+          new ListAssignee({
             id: 5,
             name: 'user5',
             username: 'user5',
             avatar: 'test_image',
-        }));
+          }),
+        );
 
         Vue.nextTick(() => done());
       });
 
       it('renders more avatar counter', () => {
-        expect(component.$el.querySelector('.board-card-assignee .avatar-counter').innerText).toEqual('+2');
+        expect(
+          component.$el.querySelector('.board-card-assignee .avatar-counter').innerText,
+        ).toEqual('+2');
       });
 
       it('renders three assignees', () => {
         expect(component.$el.querySelectorAll('.board-card-assignee .avatar').length).toEqual(3);
       });
 
-      it('renders 99+ avatar counter', (done) => {
+      it('renders 99+ avatar counter', done => {
         for (let i = 5; i < 104; i += 1) {
           const u = new ListAssignee({
             id: i,
@@ -232,7 +228,9 @@ describe('Issue card component', () => {
         }
 
         Vue.nextTick(() => {
-          expect(component.$el.querySelector('.board-card-assignee .avatar-counter').innerText).toEqual('99+');
+          expect(
+            component.$el.querySelector('.board-card-assignee .avatar-counter').innerText,
+          ).toEqual('99+');
           done();
         });
       });
@@ -240,59 +238,51 @@ describe('Issue card component', () => {
   });
 
   describe('labels', () => {
-    beforeEach((done) => {
+    beforeEach(done => {
       component.issue.addLabel(label1);
 
       Vue.nextTick(() => done());
     });
 
     it('renders list label', () => {
-      expect(
-        component.$el.querySelectorAll('.badge').length,
-      ).toBe(2);
+      expect(component.$el.querySelectorAll('.badge').length).toBe(2);
     });
 
     it('renders label', () => {
       const nodes = [];
-      component.$el.querySelectorAll('.badge').forEach((label) => {
+      component.$el.querySelectorAll('.badge').forEach(label => {
         nodes.push(label.getAttribute('data-original-title'));
       });
 
-      expect(
-        nodes.includes(label1.description),
-      ).toBe(true);
+      expect(nodes.includes(label1.description)).toBe(true);
     });
 
     it('sets label description as title', () => {
-      expect(
-        component.$el.querySelector('.badge').getAttribute('data-original-title'),
-      ).toContain(label1.description);
+      expect(component.$el.querySelector('.badge').getAttribute('data-original-title')).toContain(
+        label1.description,
+      );
     });
 
     it('sets background color of button', () => {
       const nodes = [];
-      component.$el.querySelectorAll('.badge').forEach((label) => {
+      component.$el.querySelectorAll('.badge').forEach(label => {
         nodes.push(label.style.backgroundColor);
       });
 
-      expect(
-        nodes.includes(label1.color),
-      ).toBe(true);
+      expect(nodes.includes(label1.color)).toBe(true);
     });
 
-    it('does not render label if label does not have an ID', (done) => {
-      component.issue.addLabel(new ListLabel({
+    it('does not render label if label does not have an ID', done => {
+      component.issue.addLabel(
+        new ListLabel({
           title: 'closed',
-      }));
+        }),
+      );
 
       Vue.nextTick()
         .then(() => {
-          expect(
-            component.$el.querySelectorAll('.badge').length,
-          ).toBe(2);
-          expect(
-            component.$el.textContent,
-          ).not.toContain('closed');
+          expect(component.$el.querySelectorAll('.badge').length).toBe(2);
+          expect(component.$el.textContent).not.toContain('closed');
 
           done();
         })

spec/javascripts/pipelines/pipeline_url_spec.js

@@ -35,7 +35,9 @@ describe('Pipeline Url Component', () => {
       },
     }).$mount();
 
-    expect(component.$el.querySelector('.js-pipeline-url-link').getAttribute('href')).toEqual('foo');
+    expect(component.$el.querySelector('.js-pipeline-url-link').getAttribute('href')).toEqual(
+      'foo',
+    );
     expect(component.$el.querySelector('.js-pipeline-url-link span').textContent).toEqual('#1');
   });
 
@@ -61,11 +63,11 @@ describe('Pipeline Url Component', () => {
 
     const image = component.$el.querySelector('.js-pipeline-url-user img');
 
-    expect(
-      component.$el.querySelector('.js-pipeline-url-user').getAttribute('href'),
-    ).toEqual(mockData.pipeline.user.web_url);
+    expect(component.$el.querySelector('.js-pipeline-url-user').getAttribute('href')).toEqual(
+      mockData.pipeline.user.web_url,
+    );
     expect(image.getAttribute('data-original-title')).toEqual(mockData.pipeline.user.name);
-    expect(image.getAttribute('src')).toEqual(mockData.pipeline.user.avatar_url);
+    expect(image.getAttribute('src')).toEqual(`${mockData.pipeline.user.avatar_url}?width=20`);
   });
 
   it('should render "API" when no user is provided', () => {
@@ -100,7 +102,9 @@ describe('Pipeline Url Component', () => {
     }).$mount();
 
     expect(component.$el.querySelector('.js-pipeline-url-latest').textContent).toContain('latest');
-    expect(component.$el.querySelector('.js-pipeline-url-yaml').textContent).toContain('yaml invalid');
+    expect(component.$el.querySelector('.js-pipeline-url-yaml').textContent).toContain(
+      'yaml invalid',
+    );
     expect(component.$el.querySelector('.js-pipeline-url-stuck').textContent).toContain('stuck');
   });
 
@@ -121,9 +125,9 @@ describe('Pipeline Url Component', () => {
       },
     }).$mount();
 
-    expect(
-      component.$el.querySelector('.js-pipeline-url-autodevops').textContent.trim(),
-    ).toEqual('Auto DevOps');
+    expect(component.$el.querySelector('.js-pipeline-url-autodevops').textContent.trim()).toEqual(
+      'Auto DevOps',
+    );
   });
 
   it('should render error badge when pipeline has a failure reason set', () => {
@@ -142,6 +146,8 @@ describe('Pipeline Url Component', () => {
     }).$mount();
 
     expect(component.$el.querySelector('.js-pipeline-url-failure').textContent).toContain('error');
-    expect(component.$el.querySelector('.js-pipeline-url-failure').getAttribute('data-original-title')).toContain('some reason');
+    expect(
+      component.$el.querySelector('.js-pipeline-url-failure').getAttribute('data-original-title'),
+    ).toContain('some reason');
   });
 });

spec/javascripts/vue_shared/components/notes/placeholder_note_spec.js

@@ -27,7 +27,7 @@ describe('issue placeholder system note component', () => {
         userDataMock.path,
       );
       expect(vm.$el.querySelector('.user-avatar-link img').getAttribute('src')).toEqual(
-        userDataMock.avatar_url,
+        `${userDataMock.avatar_url}?width=40`,
       );
     });
   });

spec/javascripts/vue_shared/components/user_avatar/user_avatar_image_spec.js

@@ -12,7 +12,7 @@ const DEFAULT_PROPS = {
   tooltipPlacement: 'bottom',
 };
 
-describe('User Avatar Image Component', function () {
+describe('User Avatar Image Component', function() {
   let vm;
   let UserAvatarImage;
 
@@ -20,37 +20,37 @@ describe('User Avatar Image Component', function () {
     UserAvatarImage = Vue.extend(userAvatarImage);
   });
 
-  describe('Initialization', function () {
-    beforeEach(function () {
+  describe('Initialization', function() {
+    beforeEach(function() {
       vm = mountComponent(UserAvatarImage, {
         ...DEFAULT_PROPS,
       }).$mount();
     });
 
-    it('should return a defined Vue component', function () {
+    it('should return a defined Vue component', function() {
       expect(vm).toBeDefined();
     });
 
-    it('should have <img> as a child element', function () {
+    it('should have <img> as a child element', function() {
       expect(vm.$el.tagName).toBe('IMG');
-      expect(vm.$el.getAttribute('src')).toBe(DEFAULT_PROPS.imgSrc);
-      expect(vm.$el.getAttribute('data-src')).toBe(DEFAULT_PROPS.imgSrc);
+      expect(vm.$el.getAttribute('src')).toBe(`${DEFAULT_PROPS.imgSrc}?width=99`);
+      expect(vm.$el.getAttribute('data-src')).toBe(`${DEFAULT_PROPS.imgSrc}?width=99`);
       expect(vm.$el.getAttribute('alt')).toBe(DEFAULT_PROPS.imgAlt);
     });
 
-    it('should properly compute tooltipContainer', function () {
+    it('should properly compute tooltipContainer', function() {
       expect(vm.tooltipContainer).toBe('body');
     });
 
-    it('should properly render tooltipContainer', function () {
+    it('should properly render tooltipContainer', function() {
       expect(vm.$el.getAttribute('data-container')).toBe('body');
     });
 
-    it('should properly compute avatarSizeClass', function () {
+    it('should properly compute avatarSizeClass', function() {
       expect(vm.avatarSizeClass).toBe('s99');
     });
 
-    it('should properly render img css', function () {
+    it('should properly render img css', function() {
       const { classList } = vm.$el;
       const containsAvatar = classList.contains('avatar');
       const containsSizeClass = classList.contains('s99');
@@ -64,21 +64,21 @@ describe('User Avatar Image Component', function () {
     });
   });
 
-  describe('Initialization when lazy', function () {
-    beforeEach(function () {
+  describe('Initialization when lazy', function() {
+    beforeEach(function() {
       vm = mountComponent(UserAvatarImage, {
         ...DEFAULT_PROPS,
         lazy: true,
       }).$mount();
     });
 
-    it('should add lazy attributes', function () {
+    it('should add lazy attributes', function() {
       const { classList } = vm.$el;
       const lazyClass = classList.contains('lazy');
 
       expect(lazyClass).toBe(true);
       expect(vm.$el.getAttribute('src')).toBe(placeholderImage);
-      expect(vm.$el.getAttribute('data-src')).toBe(DEFAULT_PROPS.imgSrc);
+      expect(vm.$el.getAttribute('data-src')).toBe(`${DEFAULT_PROPS.imgSrc}?width=99`);
     });
   });
 });

spec/models/concerns/avatarable_spec.rb

@@ -43,6 +43,10 @@ describe Avatarable do
         expect(project.avatar_path(only_path: only_path)).to eq(avatar_path)
       end
 
+      it 'returns the expected avatar path with width parameter' do
+        expect(project.avatar_path(only_path: only_path, size: 128)).to eq(avatar_path + "?width=128")
+      end
+
       context "when avatar is stored remotely" do
         before do
           stub_uploads_object_storage(AvatarUploader)