From 31d43e34589aaf77512203a8f9278476e3d46436 Mon Sep 17 00:00:00 2001 From: kushalpandya Date: Fri, 22 Sep 2017 21:22:19 +0530 Subject: [PATCH 1/3] Remove unnecessary use of `gsub` --- app/views/shared/issuable/_user_dropdown_item.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/shared/issuable/_user_dropdown_item.html.haml b/app/views/shared/issuable/_user_dropdown_item.html.haml index 48d04678d47..4a3547e9e70 100644 --- a/app/views/shared/issuable/_user_dropdown_item.html.haml +++ b/app/views/shared/issuable/_user_dropdown_item.html.haml @@ -4,7 +4,7 @@ %li.filter-dropdown-item{ class: ('js-current-user' if user == current_user) } %button.btn.btn-link.dropdown-user{ type: :button } .avatar-container.s40 - = user_avatar_without_link(user: user, lazy: avatar[:lazy], url: avatar[:url], size: 40, has_tooltip: false).gsub('/images/{{avatar_url}}','{{avatar_url}}').html_safe + = user_avatar_without_link(user: user, lazy: avatar[:lazy], url: avatar[:url], size: 40, has_tooltip: false) .dropdown-user-details %span = user.name From abb9981d2053ef6808ef877c0066fd53b766f06c Mon Sep 17 00:00:00 2001 From: kushalpandya Date: Fri, 22 Sep 2017 21:22:50 +0530 Subject: [PATCH 2/3] Add `data-src` when image is to be lazy loaded, use `tag` helper instead of `image_tag` helper --- app/helpers/avatars_helper.rb | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/app/helpers/avatars_helper.rb b/app/helpers/avatars_helper.rb index a4c226a6aad..be11d453898 100644 --- a/app/helpers/avatars_helper.rb +++ b/app/helpers/avatars_helper.rb @@ -13,22 +13,29 @@ module AvatarsHelper user_name = options[:user].try(:name) || options[:user_name] avatar_url = options[:url] || avatar_icon(options[:user] || options[:user_email], avatar_size) has_tooltip = options[:has_tooltip].nil? ? true : options[:has_tooltip] - data_attributes = {} + data_attributes = options[:data] || {} css_class = %W[avatar s#{avatar_size}].push(*options[:css_class]) if has_tooltip css_class.push('has-tooltip') - data_attributes = { container: 'body' } + data_attributes[:container] = 'body' end - image_tag( - avatar_url, + if options[:lazy] + css_class << 'lazy' + data_attributes[:src] = avatar_url + avatar_url = LazyImageTagHelper.placeholder_image + end + + image_options = { + alt: "#{user_name}'s avatar", + src: avatar_url, + data: data_attributes, class: css_class, - alt: "#{user_name}'s avatar", - title: user_name, - data: data_attributes, - lazy: true - ) + title: user_name + } + + tag(:img, image_options) end def user_avatar(options = {}) From d94df6ef83088b15f98a141355aae5a40c0fcbe2 Mon Sep 17 00:00:00 2001 From: kushalpandya Date: Mon, 25 Sep 2017 13:43:15 +0530 Subject: [PATCH 3/3] Update tests to reflect `user_avatar_without_link` helper changes --- spec/helpers/avatars_helper_spec.rb | 102 +++++++++++++++++----------- 1 file changed, 62 insertions(+), 40 deletions(-) diff --git a/spec/helpers/avatars_helper_spec.rb b/spec/helpers/avatars_helper_spec.rb index 4632c679972..f44e7ef6843 100644 --- a/spec/helpers/avatars_helper_spec.rb +++ b/spec/helpers/avatars_helper_spec.rb @@ -26,12 +26,13 @@ describe AvatarsHelper do subject { helper.user_avatar_without_link(options) } it 'displays user avatar' do - is_expected.to eq image_tag( - LazyImageTagHelper.placeholder_image, - class: 'avatar s16 has-tooltip lazy', + is_expected.to eq tag( + :img, alt: "#{user.name}'s avatar", - title: user.name, - data: { container: 'body', src: avatar_icon(user, 16) } + src: avatar_icon(user, 16), + data: { container: 'body' }, + class: 'avatar s16 has-tooltip', + title: user.name ) end @@ -39,12 +40,13 @@ describe AvatarsHelper do let(:options) { { user: user, css_class: '.cat-pics' } } it 'uses provided css_class' do - is_expected.to eq image_tag( - LazyImageTagHelper.placeholder_image, - class: "avatar s16 #{options[:css_class]} has-tooltip lazy", + is_expected.to eq tag( + :img, alt: "#{user.name}'s avatar", - title: user.name, - data: { container: 'body', src: avatar_icon(user, 16) } + src: avatar_icon(user, 16), + data: { container: 'body' }, + class: "avatar s16 #{options[:css_class]} has-tooltip", + title: user.name ) end end @@ -53,12 +55,13 @@ describe AvatarsHelper do let(:options) { { user: user, size: 99 } } it 'uses provided size' do - is_expected.to eq image_tag( - LazyImageTagHelper.placeholder_image, - class: "avatar s#{options[:size]} has-tooltip lazy", + is_expected.to eq tag( + :img, alt: "#{user.name}'s avatar", - title: user.name, - data: { container: 'body', src: avatar_icon(user, options[:size]) } + src: avatar_icon(user, options[:size]), + data: { container: 'body' }, + class: "avatar s#{options[:size]} has-tooltip", + title: user.name ) end end @@ -67,12 +70,28 @@ describe AvatarsHelper do let(:options) { { user: user, url: '/over/the/rainbow.png' } } it 'uses provided url' do - is_expected.to eq image_tag( - LazyImageTagHelper.placeholder_image, - class: 'avatar s16 has-tooltip lazy', + is_expected.to eq tag( + :img, alt: "#{user.name}'s avatar", - title: user.name, - data: { container: 'body', src: options[:url] } + src: options[:url], + data: { container: 'body' }, + class: "avatar s16 has-tooltip", + title: user.name + ) + end + end + + context 'with lazy parameter' do + let(:options) { { user: user, lazy: true } } + + it 'adds `lazy` class to class list, sets `data-src` with avatar URL and `src` with placeholder image' do + is_expected.to eq tag( + :img, + alt: "#{user.name}'s avatar", + src: LazyImageTagHelper.placeholder_image, + data: { container: 'body', src: avatar_icon(user, 16) }, + class: "avatar s16 has-tooltip lazy", + title: user.name ) end end @@ -82,12 +101,13 @@ describe AvatarsHelper do let(:options) { { user: user, has_tooltip: true } } it 'adds has-tooltip' do - is_expected.to eq image_tag( - LazyImageTagHelper.placeholder_image, - class: 'avatar s16 has-tooltip lazy', + is_expected.to eq tag( + :img, alt: "#{user.name}'s avatar", - title: user.name, - data: { container: 'body', src: avatar_icon(user, 16) } + src: avatar_icon(user, 16), + data: { container: 'body' }, + class: "avatar s16 has-tooltip", + title: user.name ) end end @@ -96,12 +116,12 @@ describe AvatarsHelper do let(:options) { { user: user, has_tooltip: false } } it 'does not add has-tooltip or data container' do - is_expected.to eq image_tag( - LazyImageTagHelper.placeholder_image, - class: 'avatar s16 lazy', + is_expected.to eq tag( + :img, alt: "#{user.name}'s avatar", - title: user.name, - data: { src: avatar_icon(user, 16) } + src: avatar_icon(user, 16), + class: "avatar s16", + title: user.name ) end end @@ -114,23 +134,25 @@ describe AvatarsHelper do let(:options) { { user: user, user_name: 'Tinky Winky' } } it 'prefers user parameter' do - is_expected.to eq image_tag( - LazyImageTagHelper.placeholder_image, - class: 'avatar s16 has-tooltip lazy', + is_expected.to eq tag( + :img, alt: "#{user.name}'s avatar", - title: user.name, - data: { container: 'body', src: avatar_icon(user, 16) } + src: avatar_icon(user, 16), + data: { container: 'body' }, + class: "avatar s16 has-tooltip", + title: user.name ) end end it 'uses user_name and user_email parameter if user is not present' do - is_expected.to eq image_tag( - LazyImageTagHelper.placeholder_image, - class: 'avatar s16 has-tooltip lazy', + is_expected.to eq tag( + :img, alt: "#{options[:user_name]}'s avatar", - title: options[:user_name], - data: { container: 'body', src: avatar_icon(options[:user_email], 16) } + src: avatar_icon(options[:user_email], 16), + data: { container: 'body' }, + class: "avatar s16 has-tooltip", + title: options[:user_name] ) end end