diff options
14 files changed, 136 insertions, 122 deletions
diff --git a/app/assets/javascripts/lazy_loader.js b/app/assets/javascripts/lazy_loader.js index 9482d131344..9bba341e3a3 100644 --- a/app/assets/javascripts/lazy_loader.js +++ b/app/assets/javascripts/lazy_loader.js @@ -1,6 +1,7 @@ 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'); diff --git a/app/assets/javascripts/vue_shared/components/user_avatar/user_avatar_image.vue b/app/assets/javascripts/vue_shared/components/user_avatar/user_avatar_image.vue index 3a413c74410..7737b9f2697 100644 --- a/app/assets/javascripts/vue_shared/components/user_avatar/user_avatar_image.vue +++ b/app/assets/javascripts/vue_shared/components/user_avatar/user_avatar_image.vue @@ -1,5 +1,4 @@ <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; diff --git a/app/models/concerns/avatarable.rb b/app/models/concerns/avatarable.rb index 095897b08e3..a6d604a580d 100644 --- a/app/models/concerns/avatarable.rb +++ b/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 diff --git a/app/views/admin/projects/_projects.html.haml b/app/views/admin/projects/_projects.html.haml index fdaacc098e0..50296a2afe7 100644 --- a/app/views/admin/projects/_projects.html.haml +++ b/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 diff --git a/app/views/layouts/nav/sidebar/_project.html.haml b/app/views/layouts/nav/sidebar/_project.html.haml index 2c262a2b7dd..34f47806205 100644 --- a/app/views/layouts/nav/sidebar/_project.html.haml +++ b/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 diff --git a/app/views/projects/_home_panel.html.haml b/app/views/projects/_home_panel.html.haml index 74ab8cf8250..fbe88ec9618 100644 --- a/app/views/projects/_home_panel.html.haml +++ b/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) } diff --git a/app/views/projects/edit.html.haml b/app/views/projects/edit.html.haml index 0ff88b82ae6..f483fad6142 100644 --- a/app/views/projects/edit.html.haml +++ b/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 } diff --git a/app/views/shared/projects/_project.html.haml b/app/views/shared/projects/_project.html.haml index 6be1fb485a4..be053d481e4 100644 --- a/app/views/shared/projects/_project.html.haml +++ b/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 diff --git a/spec/features/uploads/user_uploads_avatar_to_profile_spec.rb b/spec/features/uploads/user_uploads_avatar_to_profile_spec.rb index 4db73fccfb6..48f8b8bf77e 100644 --- a/spec/features/uploads/user_uploads_avatar_to_profile_spec.rb +++ b/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 diff --git a/spec/javascripts/boards/issue_card_spec.js b/spec/javascripts/boards/issue_card_spec.js index 7a32e84bced..b6c61e7bad7 100644 --- a/spec/javascripts/boards/issue_card_spec.js +++ b/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({ - id: 1, - name: 'testing 123', - username: 'test', - }, 'default_avatar')]; + beforeEach(done => { + component.issue.assignees = [ + new ListAssignee( + { + id: 1, + name: 'testing 123', + username: 'test', + }, + '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({ - id: 5, - name: 'user5', - username: 'user5', - avatar: 'test_image', - })); + 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({ - title: 'closed', - })); + 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(); }) diff --git a/spec/javascripts/pipelines/pipeline_url_spec.js b/spec/javascripts/pipelines/pipeline_url_spec.js index 4a4f2259d23..ddd580ae8b7 100644 --- a/spec/javascripts/pipelines/pipeline_url_spec.js +++ b/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'); }); }); diff --git a/spec/javascripts/vue_shared/components/notes/placeholder_note_spec.js b/spec/javascripts/vue_shared/components/notes/placeholder_note_spec.js index 7e57c51bf29..db665fdaad3 100644 --- a/spec/javascripts/vue_shared/components/notes/placeholder_note_spec.js +++ b/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`, ); }); }); diff --git a/spec/javascripts/vue_shared/components/user_avatar/user_avatar_image_spec.js b/spec/javascripts/vue_shared/components/user_avatar/user_avatar_image_spec.js index 656b57d764e..dc7652c77f7 100644 --- a/spec/javascripts/vue_shared/components/user_avatar/user_avatar_image_spec.js +++ b/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`); }); }); }); diff --git a/spec/models/concerns/avatarable_spec.rb b/spec/models/concerns/avatarable_spec.rb index 9faf21bfbbd..76f734079b7 100644 --- a/spec/models/concerns/avatarable_spec.rb +++ b/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) |