From bd21e3d7319dce4600881f7f8677b28f3f55cc5e Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 25 Dec 2015 16:41:02 +0100 Subject: [PATCH 1/8] Add Open Graph data for group, project and commit. --- app/helpers/page_layout_helper.rb | 2 ++ app/views/groups/show.html.haml | 2 ++ app/views/layouts/_head.html.haml | 10 +++++----- app/views/projects/commit/show.html.haml | 4 +++- app/views/projects/show.html.haml | 2 ++ 5 files changed, 14 insertions(+), 6 deletions(-) diff --git a/app/helpers/page_layout_helper.rb b/app/helpers/page_layout_helper.rb index 791cb9e50bd..b84644d6996 100644 --- a/app/helpers/page_layout_helper.rb +++ b/app/helpers/page_layout_helper.rb @@ -53,6 +53,8 @@ module PageLayoutHelper @project.avatar_url || default elsif @user avatar_icon(@user) + elsif @group + @group.avatar_url || default else default end diff --git a/app/views/groups/show.html.haml b/app/views/groups/show.html.haml index c2c7c581b3e..8179cdfac80 100644 --- a/app/views/groups/show.html.haml +++ b/app/views/groups/show.html.haml @@ -1,3 +1,5 @@ +- page_description @group.description + - unless can?(current_user, :read_group, @group) - @disable_search_panel = true diff --git a/app/views/layouts/_head.html.haml b/app/views/layouts/_head.html.haml index 2e0bd2007a3..2e9a34a8807 100644 --- a/app/views/layouts/_head.html.haml +++ b/app/views/layouts/_head.html.haml @@ -1,13 +1,11 @@ +- site_name = "GitLab" %head{prefix: "og: http://ogp.me/ns#"} %meta{charset: "utf-8"} %meta{'http-equiv' => 'X-UA-Compatible', content: 'IE=edge'} - %meta{name: 'referrer', content: 'origin-when-cross-origin'} - - %meta{name: "description", content: page_description} -# Open Graph - http://ogp.me/ %meta{property: 'og:type', content: "object"} - %meta{property: 'og:site_name', content: "GitLab"} + %meta{property: 'og:site_name', content: site_name} %meta{property: 'og:title', content: page_title} %meta{property: 'og:description', content: page_description} %meta{property: 'og:image', content: page_image} @@ -20,8 +18,9 @@ %meta{property: 'twitter:image', content: page_image} = page_card_meta_tags - - page_title "GitLab" + - page_title site_name %title= page_title + %meta{name: "description", content: page_description} = favicon_link_tag 'favicon.ico' @@ -34,6 +33,7 @@ = include_gon + %meta{name: 'referrer', content: 'origin-when-cross-origin'} %meta{name: 'viewport', content: 'width=device-width, initial-scale=1, maximum-scale=1'} %meta{name: 'theme-color', content: '#474D57'} diff --git a/app/views/projects/commit/show.html.haml b/app/views/projects/commit/show.html.haml index 069b8b1f169..58aa45e8d2c 100644 --- a/app/views/projects/commit/show.html.haml +++ b/app/views/projects/commit/show.html.haml @@ -1,4 +1,6 @@ -- page_title "#{@commit.title} (#{@commit.short_id})", "Commits" +- page_title "#{@commit.title} (#{@commit.short_id})", "Commits" +- page_description @commit.description + = render "projects/commits/header_title" = render "commit_box" - if @ci_commit diff --git a/app/views/projects/show.html.haml b/app/views/projects/show.html.haml index 7466a098e24..74ce005eaa2 100644 --- a/app/views/projects/show.html.haml +++ b/app/views/projects/show.html.haml @@ -1,3 +1,5 @@ +- page_description @project.description + = content_for :meta_tags do - if current_user = auto_discovery_link_tag(:atom, namespace_project_path(@project.namespace, @project, format: :atom, private_token: current_user.private_token), title: "#{@project.name} activity") From a7756a4b51b0127caa19d1fb20953cb27c4c62a8 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Sun, 27 Dec 2015 19:49:48 -0500 Subject: [PATCH 2/8] Add specs for page_image using a Group's avatar --- spec/helpers/page_layout_helper_spec.rb | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/spec/helpers/page_layout_helper_spec.rb b/spec/helpers/page_layout_helper_spec.rb index fd7107779f6..60c4eb21814 100644 --- a/spec/helpers/page_layout_helper_spec.rb +++ b/spec/helpers/page_layout_helper_spec.rb @@ -96,6 +96,22 @@ describe PageLayoutHelper do helper.page_image end end + + context 'with @group' do + it 'uses Group avatar if available' do + group = double(avatar_url: 'http://example.com/uploads/avatar.png') + helper.instance_variable_set(:@group, group) + + expect(helper.page_image).to eq group.avatar_url + end + + it 'falls back to the default' do + group = double(avatar_url: nil) + helper.instance_variable_set(:@group, group) + + expect(helper.page_image).to end_with 'assets/gitlab_logo.png' + end + end end describe 'page_card_attributes' do From dcca64a5230bbfd53ef5db8403d132deac4667f2 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Sun, 27 Dec 2015 19:58:44 -0500 Subject: [PATCH 3/8] Use `assign` instead of `instance_variable_set` --- spec/helpers/page_layout_helper_spec.rb | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/spec/helpers/page_layout_helper_spec.rb b/spec/helpers/page_layout_helper_spec.rb index 60c4eb21814..300dccf50ec 100644 --- a/spec/helpers/page_layout_helper_spec.rb +++ b/spec/helpers/page_layout_helper_spec.rb @@ -45,14 +45,14 @@ describe PageLayoutHelper do describe 'page_description_default' do it 'uses Project description when available' do project = double(description: 'Project Description') - helper.instance_variable_set(:@project, project) + assign(:project, project) expect(helper.page_description_default).to eq 'Project Description' end it 'uses brand_title when Project description is nil' do project = double(description: nil) - helper.instance_variable_set(:@project, project) + assign(:project, project) expect(helper).to receive(:brand_title).and_return('Brand Title') expect(helper.page_description_default).to eq 'Brand Title' @@ -73,14 +73,14 @@ describe PageLayoutHelper do context 'with @project' do it 'uses Project avatar if available' do project = double(avatar_url: 'http://example.com/uploads/avatar.png') - helper.instance_variable_set(:@project, project) + assign(:project, project) expect(helper.page_image).to eq project.avatar_url end it 'falls back to the default' do project = double(avatar_url: nil) - helper.instance_variable_set(:@project, project) + assign(:project, project) expect(helper.page_image).to end_with 'assets/gitlab_logo.png' end @@ -89,7 +89,7 @@ describe PageLayoutHelper do context 'with @user' do it 'delegates to avatar_icon helper' do user = double('User') - helper.instance_variable_set(:@user, user) + assign(:user, user) expect(helper).to receive(:avatar_icon).with(user) @@ -100,14 +100,14 @@ describe PageLayoutHelper do context 'with @group' do it 'uses Group avatar if available' do group = double(avatar_url: 'http://example.com/uploads/avatar.png') - helper.instance_variable_set(:@group, group) + assign(:group, group) expect(helper.page_image).to eq group.avatar_url end it 'falls back to the default' do group = double(avatar_url: nil) - helper.instance_variable_set(:@group, group) + assign(:group, group) expect(helper.page_image).to end_with 'assets/gitlab_logo.png' end From a298f694327b1241fc0d06618228e3750c20c5a1 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 5 Jan 2016 14:50:52 -0500 Subject: [PATCH 4/8] Use `User#avatar_url` instead of `avatar_icon` helper --- app/helpers/page_layout_helper.rb | 10 ++--- spec/helpers/page_layout_helper_spec.rb | 55 ++++++++----------------- 2 files changed, 22 insertions(+), 43 deletions(-) diff --git a/app/helpers/page_layout_helper.rb b/app/helpers/page_layout_helper.rb index b84644d6996..f2a4afebbd1 100644 --- a/app/helpers/page_layout_helper.rb +++ b/app/helpers/page_layout_helper.rb @@ -49,12 +49,10 @@ module PageLayoutHelper def page_image default = image_url('gitlab_logo.png') - if @project - @project.avatar_url || default - elsif @user - avatar_icon(@user) - elsif @group - @group.avatar_url || default + subject = @project || @user || @group + + if subject.present? + subject.avatar_url || default else default end diff --git a/spec/helpers/page_layout_helper_spec.rb b/spec/helpers/page_layout_helper_spec.rb index 300dccf50ec..83aeafcf31a 100644 --- a/spec/helpers/page_layout_helper_spec.rb +++ b/spec/helpers/page_layout_helper_spec.rb @@ -70,46 +70,27 @@ describe PageLayoutHelper do expect(helper.page_image).to end_with 'assets/gitlab_logo.png' end - context 'with @project' do - it 'uses Project avatar if available' do - project = double(avatar_url: 'http://example.com/uploads/avatar.png') - assign(:project, project) + %w(project user group).each do |type| + context "with @#{type} assigned" do + it "uses #{type.titlecase} avatar if available" do + object = double(avatar_url: 'http://example.com/uploads/avatar.png') + assign(type, object) - expect(helper.page_image).to eq project.avatar_url + expect(helper.page_image).to eq object.avatar_url + end + + it 'falls back to the default when avatar_url is nil' do + object = double(avatar_url: nil) + assign(type, object) + + expect(helper.page_image).to end_with 'assets/gitlab_logo.png' + end end - it 'falls back to the default' do - project = double(avatar_url: nil) - assign(:project, project) - - expect(helper.page_image).to end_with 'assets/gitlab_logo.png' - end - end - - context 'with @user' do - it 'delegates to avatar_icon helper' do - user = double('User') - assign(:user, user) - - expect(helper).to receive(:avatar_icon).with(user) - - helper.page_image - end - end - - context 'with @group' do - it 'uses Group avatar if available' do - group = double(avatar_url: 'http://example.com/uploads/avatar.png') - assign(:group, group) - - expect(helper.page_image).to eq group.avatar_url - end - - it 'falls back to the default' do - group = double(avatar_url: nil) - assign(:group, group) - - expect(helper.page_image).to end_with 'assets/gitlab_logo.png' + context "with no assignments" do + it 'falls back to the default' do + expect(helper.page_image).to end_with 'assets/gitlab_logo.png' + end end end end From 43053c2e6f03ad60f85728f36c46588979f68024 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 5 Jan 2016 14:54:59 -0500 Subject: [PATCH 5/8] Make `page_description` less magical :sparkles: --- app/helpers/page_layout_helper.rb | 12 +---------- app/views/layouts/group.html.haml | 7 ++++--- app/views/layouts/project.html.haml | 7 ++++--- spec/helpers/page_layout_helper_spec.rb | 27 ++----------------------- 4 files changed, 11 insertions(+), 42 deletions(-) diff --git a/app/helpers/page_layout_helper.rb b/app/helpers/page_layout_helper.rb index f2a4afebbd1..5c0dd36252e 100644 --- a/app/helpers/page_layout_helper.rb +++ b/app/helpers/page_layout_helper.rb @@ -27,7 +27,7 @@ module PageLayoutHelper # # Returns an HTML-safe String. def page_description(description = nil) - @page_description ||= page_description_default + @page_description ||= brand_title if description.present? @page_description = description.squish @@ -36,16 +36,6 @@ module PageLayoutHelper end end - # Default value for page_description when one hasn't been defined manually by - # a view - def page_description_default - if @project - @project.description || brand_title - else - brand_title - end - end - def page_image default = image_url('gitlab_logo.png') diff --git a/app/views/layouts/group.html.haml b/app/views/layouts/group.html.haml index 31888c5580e..1ce8d0ef7b5 100644 --- a/app/views/layouts/group.html.haml +++ b/app/views/layouts/group.html.haml @@ -1,5 +1,6 @@ -- page_title @group.name -- header_title group_title(@group) unless header_title -- sidebar "group" unless sidebar +- page_title @group.name +- page_description @group.description +- header_title group_title(@group) unless header_title +- sidebar "group" unless sidebar = render template: "layouts/application" diff --git a/app/views/layouts/project.html.haml b/app/views/layouts/project.html.haml index abf73bcc709..f81283a5ddb 100644 --- a/app/views/layouts/project.html.haml +++ b/app/views/layouts/project.html.haml @@ -1,6 +1,7 @@ -- page_title @project.name_with_namespace -- header_title project_title(@project) unless header_title -- sidebar "project" unless sidebar +- page_title @project.name_with_namespace +- page_description @project.description +- header_title project_title(@project) unless header_title +- sidebar "project" unless sidebar - content_for :scripts_body_top do - project = @target_project || @project diff --git a/spec/helpers/page_layout_helper_spec.rb b/spec/helpers/page_layout_helper_spec.rb index 83aeafcf31a..a097786ba6d 100644 --- a/spec/helpers/page_layout_helper_spec.rb +++ b/spec/helpers/page_layout_helper_spec.rb @@ -2,8 +2,8 @@ require 'rails_helper' describe PageLayoutHelper do describe 'page_description' do - it 'defaults to value returned by page_description_default helper' do - allow(helper).to receive(:page_description_default).and_return('Foo') + it 'defaults to value returned by brand_title helper' do + allow(helper).to receive(:brand_title).and_return('Foo') expect(helper.page_description).to eq 'Foo' end @@ -42,29 +42,6 @@ describe PageLayoutHelper do end end - describe 'page_description_default' do - it 'uses Project description when available' do - project = double(description: 'Project Description') - assign(:project, project) - - expect(helper.page_description_default).to eq 'Project Description' - end - - it 'uses brand_title when Project description is nil' do - project = double(description: nil) - assign(:project, project) - - expect(helper).to receive(:brand_title).and_return('Brand Title') - expect(helper.page_description_default).to eq 'Brand Title' - end - - it 'falls back to brand_title' do - allow(helper).to receive(:brand_title).and_return('Brand Title') - - expect(helper.page_description_default).to eq 'Brand Title' - end - end - describe 'page_image' do it 'defaults to the GitLab logo' do expect(helper.page_image).to end_with 'assets/gitlab_logo.png' From a0793d69c538cbb6a2b9ff4389192862f6d16962 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 5 Jan 2016 15:02:42 -0500 Subject: [PATCH 6/8] Remove now-redundant `page_description` call from Projects#show --- app/views/projects/show.html.haml | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/views/projects/show.html.haml b/app/views/projects/show.html.haml index a8f924bbb7c..8436be433b1 100644 --- a/app/views/projects/show.html.haml +++ b/app/views/projects/show.html.haml @@ -1,5 +1,3 @@ -- page_description @project.description - = content_for :meta_tags do - if current_user = auto_discovery_link_tag(:atom, namespace_project_path(@project.namespace, @project, format: :atom, private_token: current_user.private_token), title: "#{@project.name} activity") @@ -70,4 +68,4 @@ = render 'projects/last_commit', commit: @repository.commit, project: @project %div{class: "project-show-#{default_project_view}"} - = render default_project_view \ No newline at end of file + = render default_project_view From 6d3b5ea2a9611dc7d87bd48043f34f9e0930e052 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 5 Jan 2016 16:47:09 -0500 Subject: [PATCH 7/8] Remove now-redundant `page_description` call from Groups#show --- app/views/groups/show.html.haml | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/views/groups/show.html.haml b/app/views/groups/show.html.haml index e7f619d2d6b..a607d860d7d 100644 --- a/app/views/groups/show.html.haml +++ b/app/views/groups/show.html.haml @@ -1,5 +1,3 @@ -- page_description @group.description - - unless can?(current_user, :read_group, @group) - @disable_search_panel = true From 384445eca6249363c0da6d8b96e7ee030dc6fab3 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 6 Jan 2016 13:02:51 +0100 Subject: [PATCH 8/8] Don't override issue page description in project layout. --- app/helpers/page_layout_helper.rb | 11 +++-------- app/views/layouts/_head.html.haml | 2 ++ app/views/layouts/group.html.haml | 2 +- app/views/layouts/project.html.haml | 2 +- spec/helpers/page_layout_helper_spec.rb | 6 ++---- 5 files changed, 9 insertions(+), 14 deletions(-) diff --git a/app/helpers/page_layout_helper.rb b/app/helpers/page_layout_helper.rb index 5c0dd36252e..82f805fa444 100644 --- a/app/helpers/page_layout_helper.rb +++ b/app/helpers/page_layout_helper.rb @@ -27,11 +27,9 @@ module PageLayoutHelper # # Returns an HTML-safe String. def page_description(description = nil) - @page_description ||= brand_title - if description.present? @page_description = description.squish - else + elsif @page_description.present? sanitize(@page_description, tags: []).truncate_words(30) end end @@ -41,11 +39,8 @@ module PageLayoutHelper subject = @project || @user || @group - if subject.present? - subject.avatar_url || default - else - default - end + image = subject.avatar_url if subject.present? + image || default end # Define or get attributes to be used as Twitter card metadata diff --git a/app/views/layouts/_head.html.haml b/app/views/layouts/_head.html.haml index 1a2187e551b..38ca4f91c4d 100644 --- a/app/views/layouts/_head.html.haml +++ b/app/views/layouts/_head.html.haml @@ -1,3 +1,5 @@ +- page_description brand_title unless page_description + - site_name = "GitLab" %head{prefix: "og: http://ogp.me/ns#"} %meta{charset: "utf-8"} diff --git a/app/views/layouts/group.html.haml b/app/views/layouts/group.html.haml index 1ce8d0ef7b5..2e483b7148d 100644 --- a/app/views/layouts/group.html.haml +++ b/app/views/layouts/group.html.haml @@ -1,5 +1,5 @@ - page_title @group.name -- page_description @group.description +- page_description @group.description unless page_description - header_title group_title(@group) unless header_title - sidebar "group" unless sidebar diff --git a/app/views/layouts/project.html.haml b/app/views/layouts/project.html.haml index f81283a5ddb..ab527e8e438 100644 --- a/app/views/layouts/project.html.haml +++ b/app/views/layouts/project.html.haml @@ -1,5 +1,5 @@ - page_title @project.name_with_namespace -- page_description @project.description +- page_description @project.description unless page_description - header_title project_title(@project) unless header_title - sidebar "project" unless sidebar diff --git a/spec/helpers/page_layout_helper_spec.rb b/spec/helpers/page_layout_helper_spec.rb index a097786ba6d..cf632f594c7 100644 --- a/spec/helpers/page_layout_helper_spec.rb +++ b/spec/helpers/page_layout_helper_spec.rb @@ -2,10 +2,8 @@ require 'rails_helper' describe PageLayoutHelper do describe 'page_description' do - it 'defaults to value returned by brand_title helper' do - allow(helper).to receive(:brand_title).and_return('Foo') - - expect(helper.page_description).to eq 'Foo' + it 'defaults to nil' do + expect(helper.page_description).to eq nil end it 'returns the last-pushed description' do