From c0242485393fe93397ee18889bc5345b67d5ea0d Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 22 Feb 2017 19:27:06 -0600 Subject: Update permalink/blame buttons with line number fragment hash Fix https://gitlab.com/gitlab-org/gitlab-ce/issues/19742 --- .../blob/blob_line_permalink_updater.js | 35 ++++++++ app/assets/javascripts/dispatcher.js | 8 ++ app/assets/javascripts/line_highlighter.js | 12 +-- app/assets/stylesheets/framework/highlight.scss | 9 +- app/views/projects/blob/_actions.html.haml | 2 +- ...rmalink-blame-button-line-number-hash-links.yml | 4 + .../blobs/blob_line_permalink_updater_spec.rb | 97 ++++++++++++++++++++++ spec/javascripts/line_highlighter_spec.js | 18 +--- 8 files changed, 157 insertions(+), 28 deletions(-) create mode 100644 app/assets/javascripts/blob/blob_line_permalink_updater.js create mode 100644 changelogs/unreleased/19742-permalink-blame-button-line-number-hash-links.yml create mode 100644 spec/features/projects/blobs/blob_line_permalink_updater_spec.rb diff --git a/app/assets/javascripts/blob/blob_line_permalink_updater.js b/app/assets/javascripts/blob/blob_line_permalink_updater.js new file mode 100644 index 00000000000..c8f68860fbd --- /dev/null +++ b/app/assets/javascripts/blob/blob_line_permalink_updater.js @@ -0,0 +1,35 @@ +const lineNumberRe = /^L[0-9]+/; + +const updateLineNumbersOnBlobPermalinks = (linksToUpdate) => { + const hash = gl.utils.getLocationHash(); + if (hash && lineNumberRe.test(hash)) { + const hashUrlString = `#${hash}`; + + [].concat(Array.prototype.slice.call(linksToUpdate)).forEach((permalinkButton) => { + const baseHref = permalinkButton.getAttribute('data-original-href') || (() => { + const href = permalinkButton.getAttribute('href'); + permalinkButton.setAttribute('data-original-href', href); + return href; + })(); + permalinkButton.setAttribute('href', `${baseHref}${hashUrlString}`); + }); + } +}; + +function BlobLinePermalinkUpdater(blobContentHolder, lineNumberSelector, elementsToUpdate) { + const updateBlameAndBlobPermalinkCb = () => { + // Wait for the hash to update from the LineHighlighter callback + setTimeout(() => { + updateLineNumbersOnBlobPermalinks(elementsToUpdate); + }, 0); + }; + + blobContentHolder.addEventListener('click', (e) => { + if (e.target.matches(lineNumberSelector)) { + updateBlameAndBlobPermalinkCb(); + } + }); + updateBlameAndBlobPermalinkCb(); +} + +export default BlobLinePermalinkUpdater; diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js index 7b9b9123c31..7956d6afff2 100644 --- a/app/assets/javascripts/dispatcher.js +++ b/app/assets/javascripts/dispatcher.js @@ -40,6 +40,7 @@ import BindInOut from './behaviors/bind_in_out'; import GroupsList from './groups_list'; import ProjectsList from './projects_list'; import MiniPipelineGraph from './mini_pipeline_graph_dropdown'; +import BlobLinePermalinkUpdater from './blob/blob_line_permalink_updater'; const ShortcutsBlob = require('./shortcuts_blob'); const UserCallout = require('./user_callout'); @@ -252,6 +253,13 @@ const UserCallout = require('./user_callout'); case 'projects:blob:show': case 'projects:blame:show': new LineHighlighter(); + + new BlobLinePermalinkUpdater( + document.querySelector('#blob-content-holder'), + '.diff-line-num[data-line-number]', + document.querySelectorAll('.js-data-file-blob-permalink-url, .js-blob-blame-link'), + ); + shortcut_handler = new ShortcutsNavigation(); const fileBlobPermalinkUrlElement = document.querySelector('.js-data-file-blob-permalink-url'); const fileBlobPermalinkUrl = fileBlobPermalinkUrlElement && fileBlobPermalinkUrlElement.getAttribute('href'); diff --git a/app/assets/javascripts/line_highlighter.js b/app/assets/javascripts/line_highlighter.js index 966fcd8ec47..1821ca18053 100644 --- a/app/assets/javascripts/line_highlighter.js +++ b/app/assets/javascripts/line_highlighter.js @@ -67,17 +67,7 @@ require('vendor/jquery.scrollTo'); } LineHighlighter.prototype.bindEvents = function() { - $('#blob-content-holder').on('mousedown', 'a[data-line-number]', this.clickHandler); - // While it may seem odd to bind to the mousedown event and then throw away - // the click event, there is a method to our madness. - // - // If not done this way, the line number anchor will sometimes keep its - // active state even when the event is cancelled, resulting in an ugly border - // around the link and/or a persisted underline text decoration. - $('#blob-content-holder').on('click', 'a[data-line-number]', function(event) { - event.preventDefault(); - event.stopPropagation(); - }); + $('#blob-content-holder').on('click', 'a[data-line-number]', this.clickHandler); }; LineHighlighter.prototype.clickHandler = function(event) { diff --git a/app/assets/stylesheets/framework/highlight.scss b/app/assets/stylesheets/framework/highlight.scss index 909a0f4afda..6d27d7568cf 100644 --- a/app/assets/stylesheets/framework/highlight.scss +++ b/app/assets/stylesheets/framework/highlight.scss @@ -57,8 +57,13 @@ visibility: hidden; } - &:hover i { - visibility: visible; + &:hover, + &:focus { + outline: none; + + & i { + visibility: visible; + } } } } diff --git a/app/views/projects/blob/_actions.html.haml b/app/views/projects/blob/_actions.html.haml index c44d8fcd430..14d42f7d9ec 100644 --- a/app/views/projects/blob/_actions.html.haml +++ b/app/views/projects/blob/_actions.html.haml @@ -12,7 +12,7 @@ class: 'btn btn-sm' - else = link_to 'Blame', namespace_project_blame_path(@project.namespace, @project, @id), - class: 'btn btn-sm' unless @blob.empty? + class: 'btn btn-sm js-blob-blame-link' unless @blob.empty? = link_to 'History', namespace_project_commits_path(@project.namespace, @project, @id), class: 'btn btn-sm' = link_to 'Permalink', namespace_project_blob_path(@project.namespace, @project, diff --git a/changelogs/unreleased/19742-permalink-blame-button-line-number-hash-links.yml b/changelogs/unreleased/19742-permalink-blame-button-line-number-hash-links.yml new file mode 100644 index 00000000000..199f1edec8b --- /dev/null +++ b/changelogs/unreleased/19742-permalink-blame-button-line-number-hash-links.yml @@ -0,0 +1,4 @@ +--- +title: Update permalink/blame buttons with line number fragment hash +merge_request: +author: diff --git a/spec/features/projects/blobs/blob_line_permalink_updater_spec.rb b/spec/features/projects/blobs/blob_line_permalink_updater_spec.rb new file mode 100644 index 00000000000..d94204230f6 --- /dev/null +++ b/spec/features/projects/blobs/blob_line_permalink_updater_spec.rb @@ -0,0 +1,97 @@ +require 'spec_helper' + +feature 'Blob button line permalinks (BlobLinePermalinkUpdater)', feature: true, js: true do + include TreeHelper + + let(:project) { create(:project, :public, :repository) } + let(:path) { 'CHANGELOG' } + let(:sha) { project.repository.commit.sha } + + describe 'On a file(blob)' do + def get_absolute_url(path = "") + "http://#{page.server.host}:#{page.server.port}#{path}" + end + + def visit_blob(fragment = nil) + visit namespace_project_blob_path(project.namespace, project, tree_join('master', path), anchor: fragment) + end + + describe 'Click "Permalink" button' do + it 'works with no initial line number fragment hash' do + visit_blob + + expect(find('.js-data-file-blob-permalink-url')['href']).to eq(get_absolute_url(namespace_project_blob_path(project.namespace, project, tree_join(sha, path)))) + end + + it 'maintains intitial fragment hash' do + fragment = "L3" + + visit_blob(fragment) + + expect(find('.js-data-file-blob-permalink-url')['href']).to eq(get_absolute_url(namespace_project_blob_path(project.namespace, project, tree_join(sha, path), anchor: fragment))) + end + + it 'changes fragment hash if line number clicked' do + ending_fragment = "L5" + + visit_blob + + find('#L3').click + find("##{ending_fragment}").click + + expect(find('.js-data-file-blob-permalink-url')['href']).to eq(get_absolute_url(namespace_project_blob_path(project.namespace, project, tree_join(sha, path), anchor: ending_fragment))) + end + + it 'with initial fragment hash, changes fragment hash if line number clicked' do + fragment = "L1" + ending_fragment = "L5" + + visit_blob(fragment) + + find('#L3').click + find("##{ending_fragment}").click + + expect(find('.js-data-file-blob-permalink-url')['href']).to eq(get_absolute_url(namespace_project_blob_path(project.namespace, project, tree_join(sha, path), anchor: ending_fragment))) + end + end + + describe 'Click "Blame" button' do + it 'works with no initial line number fragment hash' do + visit_blob + + expect(find('.js-blob-blame-link')['href']).to eq(get_absolute_url(namespace_project_blame_path(project.namespace, project, tree_join('master', path)))) + end + + it 'maintains intitial fragment hash' do + fragment = "L3" + + visit_blob(fragment) + + expect(find('.js-blob-blame-link')['href']).to eq(get_absolute_url(namespace_project_blame_path(project.namespace, project, tree_join('master', path), anchor: fragment))) + end + + it 'changes fragment hash if line number clicked' do + ending_fragment = "L5" + + visit_blob + + find('#L3').click + find("##{ending_fragment}").click + + expect(find('.js-blob-blame-link')['href']).to eq(get_absolute_url(namespace_project_blame_path(project.namespace, project, tree_join('master', path), anchor: ending_fragment))) + end + + it 'with initial fragment hash, changes fragment hash if line number clicked' do + fragment = "L1" + ending_fragment = "L5" + + visit_blob(fragment) + + find('#L3').click + find("##{ending_fragment}").click + + expect(find('.js-blob-blame-link')['href']).to eq(get_absolute_url(namespace_project_blame_path(project.namespace, project, tree_join('master', path), anchor: ending_fragment))) + end + end + end +end diff --git a/spec/javascripts/line_highlighter_spec.js b/spec/javascripts/line_highlighter_spec.js index a0b2ebc221b..a1fd2d38968 100644 --- a/spec/javascripts/line_highlighter_spec.js +++ b/spec/javascripts/line_highlighter_spec.js @@ -7,16 +7,12 @@ require('~/line_highlighter'); describe('LineHighlighter', function() { var clickLine; preloadFixtures('static/line_highlighter.html.raw'); - clickLine = function(number, eventData) { - var e; - if (eventData == null) { - eventData = {}; - } + clickLine = function(number, eventData = {}) { if ($.isEmptyObject(eventData)) { - return $("#L" + number).mousedown().click(); + return $("#L" + number).click(); } else { - e = $.Event('mousedown', eventData); - return $("#L" + number).trigger(e).click(); + const e = $.Event('click', eventData); + return $("#L" + number).trigger(e); } }; beforeEach(function() { @@ -63,12 +59,6 @@ require('~/line_highlighter'); }); }); describe('#clickHandler', function() { - it('discards the mousedown event', function() { - var spy; - spy = spyOnEvent('a[data-line-number]', 'mousedown'); - clickLine(13); - return expect(spy).toHaveBeenPrevented(); - }); it('handles clicking on a child icon element', function() { var spy; spy = spyOn(this["class"], 'setHash').and.callThrough(); -- cgit v1.2.1