Implement review comments

Changes include:
- Use GlSprintf component for i18n to prevent encoding of special
characters
- Improve readability of RSpec tests
- Convert `.cover-controls` to utility classes

app/assets/javascripts/vue_shared/components/user_popover/user_popover.vue

 <script>
-import { GlPopover, GlSkeletonLoading } from '@gitlab/ui';
+import { GlPopover, GlSkeletonLoading, GlSprintf } from '@gitlab/ui';
 import Icon from '~/vue_shared/components/icon.vue';
 import UserAvatarImage from '../user_avatar/user_avatar_image.vue';
 import { glEmojiTag } from '../../../emoji';
-import { s__, sprintf } from '~/locale';
+import { s__ } from '~/locale';
+import { isString } from 'lodash';
 
 export default {
   name: 'UserPopover',
@@ -11,6 +12,7 @@ export default {
     Icon,
     GlPopover,
     GlSkeletonLoading,
+    GlSprintf,
     UserAvatarImage,
   },
   props: {
@@ -53,7 +55,10 @@ export default {
       const { jobTitle, organization } = this.user;
 
       if (organization && jobTitle) {
-        return sprintf(s__('Profile|%{jobTitle} at %{organization}'), { jobTitle, organization });
+        return {
+          message: s__('Profile|%{jobTitle} at %{organization}'),
+          placeholders: { organization, jobTitle },
+        };
       } else if (organization) {
         return organization;
       } else if (jobTitle) {
@@ -62,6 +67,9 @@ export default {
 
       return null;
     },
+    workInformationShouldUseSprintf() {
+      return !isString(this.workInformation);
+    },
     locationIsLoading() {
       return !this.user.loaded && this.user.location === null;
     },
@@ -86,17 +94,27 @@ export default {
           <gl-skeleton-loading v-else :lines="1" class="animation-container-small mb-1" />
         </div>
         <div class="text-secondary">
-          <div v-if="user.bio" class="js-bio d-flex mb-1">
+          <div v-if="user.bio" class="d-flex mb-1">
             <icon name="profile" class="category-icon flex-shrink-0" />
-            <span class="ml-1">{{ user.bio }}</span>
+            <span ref="bio" class="ml-1">{{ user.bio }}</span>
           </div>
-          <div v-if="workInformation" class="js-work-information d-flex mb-1">
+          <div v-if="workInformation" class="d-flex mb-1">
             <icon
               v-show="!workInformationIsLoading"
               name="work"
               class="category-icon flex-shrink-0"
             />
-            <span class="ml-1">{{ workInformation }}</span>
+            <span ref="workInformation" class="ml-1">
+              <gl-sprintf v-if="workInformationShouldUseSprintf" :message="workInformation.message">
+                <template
+                  v-for="(placeholder, slotName) in workInformation.placeholders"
+                  v-slot:[slotName]
+                >
+                  <span :key="slotName">{{ placeholder }}</span>
+                </template>
+              </gl-sprintf>
+              <span v-else>{{ workInformation }}</span>
+            </span>
           </div>
           <gl-skeleton-loading
             v-if="workInformationIsLoading"

app/assets/stylesheets/framework/blocks.scss

@@ -161,13 +161,17 @@
   }
 
   .cover-controls {
-    position: absolute;
-    top: 10px;
-    right: 10px;
+    @include media-breakpoint-up(sm) {
+      position: absolute;
+      top: 1rem;
+      right: 1.25rem;
+    }
 
     &.left {
-      left: 10px;
-      right: auto;
+      @include media-breakpoint-up(sm) {
+        left: 1.25rem;
+        right: auto;
+      }
     }
   }
 

app/assets/stylesheets/pages/profile.scss

@@ -197,23 +197,6 @@
 }
 
 .user-profile {
-  .cover-controls {
-    @include media-breakpoint-down(xs) {
-      position: static;
-      display: flex;
-      padding: 0 0.875rem 1.25rem;
-    }
-
-    a {
-      margin-left: 0.25rem;
-
-      @include media-breakpoint-down(xs) {
-        margin: 0 0.125rem;
-        flex: 1 0 auto;
-      }
-    }
-  }
-
   .profile-header {
     margin: 0 $gl-padding;
 

app/views/help/ui.html.haml

 - page_title "UI Development Kit", "Help"
 - lorem = "Lorem ipsum dolor sit amet, consectetur adipiscing elit. Sed fermentum nisi sapien, non consequat lectus aliquam ultrices. Suspendisse sodales est euismod nunc condimentum, a consectetur diam ornare."
+- link_classes = "flex-grow-1 mx-1 "
 
 .gitlab-ui-dev-kit
   %h1 GitLab UI development kit
@@ -64,7 +65,12 @@
     Cover block for profile page with avatar, name and description
     %code .cover-block
   .example
-    .cover-block
+    .cover-block.user-cover-block
+      = render layout: 'users/cover_controls' do
+        = link_to '#', class: link_classes + 'btn btn-default' do
+          = icon('pencil')
+        = link_to '#', class: link_classes + 'btn btn-default' do
+          = icon('rss')
       .avatar-holder
         = image_tag avatar_icon_for_email('admin@example.com', 90), class: "avatar s90", alt: ''
       .cover-title
@@ -73,13 +79,6 @@
       .cover-desc.cgray
         = lorem
 
-      .cover-controls
-        = link_to '#', class: 'btn btn-default' do
-          = icon('pencil')
-         
-        = link_to '#', class: 'btn btn-default' do
-          = icon('rss')
-
   %h2#lists Lists
 
   .lead

app/views/users/_cover_controls.html.haml

+.cover-controls.d-flex.px-2.pb-4.d-sm-block.p-sm-0
+  = yield

app/views/users/show.html.haml

@@ -4,30 +4,31 @@
 - page_title       @user.blocked? ? s_('UserProfile|Blocked user') : @user.name
 - page_description @user.bio
 - header_title     @user.name, user_path(@user)
+- link_classes = "flex-grow-1 mx-1 "
 
 = content_for :meta_tags do
   = auto_discovery_link_tag(:atom, user_url(@user, format: :atom), title: "#{@user.name} activity")
 
 .user-profile
   .cover-block.user-cover-block{ class: [('border-bottom' if profile_tabs.empty?)] }
-    .cover-controls
+    = render layout: 'users/cover_controls' do
       - if @user == current_user
-        = link_to profile_path, class: 'btn btn-default has-tooltip', title: s_('UserProfile|Edit profile'), 'aria-label': 'Edit profile' do
+        = link_to profile_path, class: link_classes + 'btn btn-default has-tooltip', title: s_('UserProfile|Edit profile'), 'aria-label': 'Edit profile' do
           = icon('pencil')
       - elsif current_user
         - if @user.abuse_report
-          %button.btn.btn-danger{ title: s_('UserProfile|Already reported for abuse'),
+          %button{ class: link_classes + 'btn btn-danger mr-1', title: s_('UserProfile|Already reported for abuse'),
             data: { toggle: 'tooltip', placement: 'bottom', container: 'body' } }
             = icon('exclamation-circle')
         - else
-          = link_to new_abuse_report_path(user_id: @user.id, ref_url: request.referrer), class: 'btn',
+          = link_to new_abuse_report_path(user_id: @user.id, ref_url: request.referrer), class: link_classes + 'btn',
             title: s_('UserProfile|Report abuse'), data: { toggle: 'tooltip', placement: 'bottom', container: 'body' } do
             = icon('exclamation-circle')
       - if can?(current_user, :read_user_profile, @user)
-        = link_to user_path(@user, rss_url_options), class: 'btn btn-default has-tooltip', title: s_('UserProfile|Subscribe'), 'aria-label': 'Subscribe' do
+        = link_to user_path(@user, rss_url_options), class: link_classes + 'btn btn-default has-tooltip', title: s_('UserProfile|Subscribe'), 'aria-label': 'Subscribe' do
           = icon('rss')
       - if current_user && current_user.admin?
-        = link_to [:admin, @user], class: 'btn btn-default', title: s_('UserProfile|View user in admin area'),
+        = link_to [:admin, @user], class: link_classes + 'btn btn-default', title: s_('UserProfile|View user in admin area'),
           data: {toggle: 'tooltip', placement: 'bottom', container: 'body'} do
           = icon('users')
 

spec/features/profiles/user_edit_profile_spec.rb

@@ -383,32 +383,38 @@ describe 'User edit profile' do
   end
 
   context 'work information', :js do
-    it 'shows user\'s job title and organization when both entered' do
-      fill_in 'user_job_title', with: 'Frontend Engineer'
-      fill_in 'user_organization', with: 'GitLab - work info test'
-      submit_settings
+    context 'when job title and organziation are entered' do
+      it "shows job title and organzation on user's profile" do
+        fill_in 'user_job_title', with: 'Frontend Engineer'
+        fill_in 'user_organization', with: 'GitLab - work info test'
+        submit_settings
 
-      visit_user
+        visit_user
 
-      expect(page).to have_content('Frontend Engineer at GitLab - work info test')
+        expect(page).to have_content('Frontend Engineer at GitLab - work info test')
+      end
     end
 
-    it 'shows user\'s job title when only job title is entered' do
-      fill_in 'user_job_title', with: 'Frontend Engineer - work info test'
-      submit_settings
+    context 'when only job title is entered' do
+      it "shows only job title on user's profile" do
+        fill_in 'user_job_title', with: 'Frontend Engineer - work info test'
+        submit_settings
 
-      visit_user
+        visit_user
 
-      expect(page).to have_content('Frontend Engineer - work info test')
+        expect(page).to have_content('Frontend Engineer - work info test')
+      end
     end
 
-    it 'shows user\'s organization when only organization is entered' do
-      fill_in 'user_organization', with: 'GitLab - work info test'
-      submit_settings
+    context 'when only organization is entered' do
+      it "shows only organization on user's profile" do
+        fill_in 'user_organization', with: 'GitLab - work info test'
+        submit_settings
 
-      visit_user
+        visit_user
 
-      expect(page).to have_content('GitLab - work info test')
+        expect(page).to have_content('GitLab - work info test')
+      end
     end
   end
 end

spec/frontend/vue_shared/components/user_popover/user_popover_spec.js

-import { GlSkeletonLoading } from '@gitlab/ui';
+import { GlSkeletonLoading, GlSprintf } from '@gitlab/ui';
 import { shallowMount } from '@vue/test-utils';
 import UserPopover from '~/vue_shared/components/user_popover/user_popover.vue';
 import Icon from '~/vue_shared/components/icon.vue';
@@ -40,6 +40,9 @@ describe('User Popover Component', () => {
         target: findTarget(),
         ...props,
       },
+      stubs: {
+        'gl-sprintf': GlSprintf,
+      },
       ...options,
     });
   };
@@ -87,13 +90,16 @@ describe('User Popover Component', () => {
   });
 
   describe('job data', () => {
+    const findWorkInformation = () => wrapper.find({ ref: 'workInformation' });
+    const findBio = () => wrapper.find({ ref: 'bio' });
+
     it('should show only bio if organization and job title are not available', () => {
-      const user = { ...DEFAULT_PROPS.user, bio: 'Engineer' };
+      const user = { ...DEFAULT_PROPS.user, bio: 'My super interesting bio' };
 
       createWrapper({ user });
 
-      expect(wrapper.text()).toContain('Engineer');
-      expect(wrapper.find('.js-work-information').exists()).toBe(false);
+      expect(findBio().text()).toBe('My super interesting bio');
+      expect(findWorkInformation().exists()).toBe(false);
     });
 
     it('should show only organization if job title is not available', () => {
@@ -101,7 +107,7 @@ describe('User Popover Component', () => {
 
       createWrapper({ user });
 
-      expect(wrapper.find('.js-work-information > span').text()).toBe('GitLab');
+      expect(findWorkInformation().text()).toBe('GitLab');
     });
 
     it('should show only job title if organization is not available', () => {
@@ -109,7 +115,7 @@ describe('User Popover Component', () => {
 
       createWrapper({ user });
 
-      expect(wrapper.find('.js-work-information > span').text()).toBe('Frontend Engineer');
+      expect(findWorkInformation().text()).toBe('Frontend Engineer');
     });
 
     it('should show organization and job title if they are both available', () => {
@@ -121,40 +127,88 @@ describe('User Popover Component', () => {
 
       createWrapper({ user });
 
-      expect(wrapper.find('.js-work-information > span').text()).toBe(
-        'Frontend Engineer at GitLab',
-      );
+      expect(findWorkInformation().text()).toBe('Frontend Engineer at GitLab');
     });
 
     it('should display bio and job info in separate lines', () => {
-      const user = { ...DEFAULT_PROPS.user, bio: 'Engineer', organization: 'GitLab' };
+      const user = {
+        ...DEFAULT_PROPS.user,
+        bio: 'My super interesting bio',
+        organization: 'GitLab',
+      };
 
       createWrapper({ user });
 
-      expect(wrapper.find('.js-bio').text()).toContain('Engineer');
-      expect(wrapper.find('.js-work-information').text()).toContain('GitLab');
+      expect(findBio().text()).toBe('My super interesting bio');
+      expect(findWorkInformation().text()).toBe('GitLab');
     });
 
-    it('should not encode special characters in bio and organization', () => {
+    it('should not encode special characters in bio', () => {
+      const user = {
+        ...DEFAULT_PROPS.user,
+        bio: 'I like <html> & CSS',
+      };
+
+      createWrapper({ user });
+
+      expect(findBio().text()).toBe('I like <html> & CSS');
+    });
+
+    it('should not encode special characters in organization', () => {
       const user = {
         ...DEFAULT_PROPS.user,
-        bio: 'Manager & Team Lead',
         organization: 'Me & my <funky> Company',
       };
 
       createWrapper({ user });
 
-      expect(wrapper.find('.js-bio').text()).toContain('Manager & Team Lead');
-      expect(wrapper.find('.js-work-information').text()).toContain('Me & my <funky> Company');
+      expect(findWorkInformation().text()).toBe('Me & my <funky> Company');
+    });
+
+    it('should not encode special characters in job title', () => {
+      const user = {
+        ...DEFAULT_PROPS.user,
+        jobTitle: 'Manager & Team Lead',
+      };
+
+      createWrapper({ user });
+
+      expect(findWorkInformation().text()).toBe('Manager & Team Lead');
+    });
+
+    it('should not encode special characters when both job title and organization are set', () => {
+      const user = {
+        ...DEFAULT_PROPS.user,
+        jobTitle: 'Manager & Team Lead',
+        organization: 'Me & my <funky> Company',
+      };
+
+      createWrapper({ user });
+
+      expect(findWorkInformation().text()).toBe('Manager & Team Lead at Me & my <funky> Company');
     });
 
     it('shows icon for bio', () => {
+      const user = {
+        ...DEFAULT_PROPS.user,
+        bio: 'My super interesting bio',
+      };
+
+      createWrapper({ user });
+
       expect(wrapper.findAll(Icon).filter(icon => icon.props('name') === 'profile').length).toEqual(
         1,
       );
     });
 
     it('shows icon for organization', () => {
+      const user = {
+        ...DEFAULT_PROPS.user,
+        organization: 'GitLab',
+      };
+
+      createWrapper({ user });
+
       expect(wrapper.findAll(Icon).filter(icon => icon.props('name') === 'work').length).toEqual(1);
     });
   });

spec/helpers/users_helper_spec.rb

@@ -180,27 +180,40 @@ describe UsersHelper do
   end
 
   describe '#work_information' do
-    it "returns job title concatinated with organization if both are present" do
-      user = create(:user, organization: 'GitLab', job_title: 'Frontend Engineer')
-      expect(helper.work_information(user)).to eq('Frontend Engineer at GitLab')
+    subject { helper.work_information(user) }
+
+    context 'when both job_title and organization are present' do
+      let(:user) { create(:user, organization: 'GitLab', job_title: 'Frontend Engineer') }
+
+      it 'returns job title concatinated with organization' do
+        is_expected.to eq('Frontend Engineer at GitLab')
+      end
     end
 
-    it "returns organization if only organization is present" do
-      user = create(:user, organization: 'GitLab')
-      expect(helper.work_information(user)).to eq('GitLab')
+    context 'when only organization is present' do
+      let(:user) { create(:user, organization: 'GitLab') }
+
+      it "returns organization" do
+        is_expected.to eq('GitLab')
+      end
     end
 
-    it "returns job title if only job_title is present" do
-      user = create(:user, job_title: 'Frontend Engineer')
-      expect(helper.work_information(user)).to eq('Frontend Engineer')
+    context 'when only job_title is present' do
+      let(:user) { create(:user, job_title: 'Frontend Engineer') }
+
+      it 'returns job title' do
+        is_expected.to eq('Frontend Engineer')
+      end
     end
 
-    it "returns nil if job_title and organization are not present" do
-      expect(helper.work_information(user)).to be_nil
+    context 'when neither organization nor job_title are present' do
+      it { is_expected.to be_nil }
     end
 
-    it "returns nil user paramater is nil" do
-      expect(helper.work_information(nil)).to be_nil
+    context 'when user parameter is nil' do
+      let(:user) { nil }
+
+      it { is_expected.to be_nil }
     end
   end
 end