From 429eb466ea910102cb402792ee12661dd9977215 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Thu, 9 Mar 2017 13:03:08 +0000 Subject: [PATCH 1/5] Prevent dropdown from closing when user clicks in a build. --- .../javascripts/mini_pipeline_graph_dropdown.js | 16 ++++++++++++++++ .../mini_pipeline_graph_dropdown_spec.js | 15 +++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/app/assets/javascripts/mini_pipeline_graph_dropdown.js b/app/assets/javascripts/mini_pipeline_graph_dropdown.js index 2145e531331..483c201305a 100644 --- a/app/assets/javascripts/mini_pipeline_graph_dropdown.js +++ b/app/assets/javascripts/mini_pipeline_graph_dropdown.js @@ -21,6 +21,8 @@ this.container = opts.container || ''; this.dropdownListSelector = '.js-builds-dropdown-container'; this.getBuildsList = this.getBuildsList.bind(this); + + this.stopDropdownClickPropagation(); } /** @@ -31,6 +33,20 @@ $(document).off('shown.bs.dropdown', this.container).on('shown.bs.dropdown', this.container, this.getBuildsList); } + /** + * When the user right clicks or cmd/ctrl + click in the job name + * the dropdown should not be closed and the link should open in another tab, + * so we stop propagation of the click event inside the dropdown. + * + * Since this component is rendered multiple times per page we need to guarantee we only + * target the click event of this component. + */ + stopDropdownClickPropagation() { + $(document).on('click', `${this.container} .js-builds-dropdown-list a.mini-pipeline-graph-dropdown-item`, (e) => { + e.stopPropagation(); + }); + } + /** * For the clicked stage, renders the given data in the dropdown list. * diff --git a/spec/javascripts/mini_pipeline_graph_dropdown_spec.js b/spec/javascripts/mini_pipeline_graph_dropdown_spec.js index 7cdade01e00..f6b3dc87cd8 100644 --- a/spec/javascripts/mini_pipeline_graph_dropdown_spec.js +++ b/spec/javascripts/mini_pipeline_graph_dropdown_spec.js @@ -46,6 +46,21 @@ require('~/mini_pipeline_graph_dropdown'); document.querySelector('.js-builds-dropdown-button').click(); expect(ajaxSpy.calls.allArgs()[0][0].url).toEqual('foobar'); }); + + it('should not close when user uses cmd/ctrl + click', () => { + spyOn($, 'ajax').and.callFake(function (params) { + params.success({ + html: '\u003cli\u003e\n\u003ca class="mini-pipeline-graph-dropdown-item" href="#"\u003e\u003cspan class="ci-status-icon ci-status-icon-failed"\u003e\u003c/span\u003e\n\u003cspan class="ci-build-text"\u003ebuild\u003c/span\u003e\n\u003c/a\u003e\u003ca class="ci-action-icon-wrapper js-ci-action-icon" href="#"\u003e\u003c/a\u003e\n\u003c/li\u003e\n', + }); + }); + new gl.MiniPipelineGraph({ container: '.js-builds-dropdown-tests' }).bindEvents(); + + document.querySelector('.js-builds-dropdown-button').click(); + + document.querySelector('a.mini-pipeline-graph-dropdown-item').click(); + + expect($('.js-builds-dropdown-list').is(':visible')).toEqual(true); + }); }); }); })(); From 5a3b2f83b09a58d487a85676563b38008bd2ff86 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Thu, 9 Mar 2017 18:26:15 +0000 Subject: [PATCH 2/5] Uses vanilla JS to listen to click event Removes `:visible` from selector Adds CHANGELOG entry Removes iife --- app/assets/javascripts/dispatcher.js | 3 +- .../javascripts/merge_request_widget.js | 5 +- .../mini_pipeline_graph_dropdown.js | 177 +++++++++--------- .../24166-close-builds-dropdown.yml | 4 + .../mini_pipeline_graph_dropdown_spec.js | 16 +- 5 files changed, 103 insertions(+), 102 deletions(-) create mode 100644 changelogs/unreleased/24166-close-builds-dropdown.yml diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js index 017980271b1..7b9b9123c31 100644 --- a/app/assets/javascripts/dispatcher.js +++ b/app/assets/javascripts/dispatcher.js @@ -39,6 +39,7 @@ import Issue from './issue'; import BindInOut from './behaviors/bind_in_out'; import GroupsList from './groups_list'; import ProjectsList from './projects_list'; +import MiniPipelineGraph from './mini_pipeline_graph_dropdown'; const ShortcutsBlob = require('./shortcuts_blob'); const UserCallout = require('./user_callout'); @@ -181,7 +182,7 @@ const UserCallout = require('./user_callout'); shortcut_handler = new ShortcutsNavigation(); break; case 'projects:commit:pipelines': - new gl.MiniPipelineGraph({ + new MiniPipelineGraph({ container: '.js-pipeline-table', }).bindEvents(); break; diff --git a/app/assets/javascripts/merge_request_widget.js b/app/assets/javascripts/merge_request_widget.js index 5f1bd474a0c..a80ae128fa4 100644 --- a/app/assets/javascripts/merge_request_widget.js +++ b/app/assets/javascripts/merge_request_widget.js @@ -4,6 +4,7 @@ /* global merge_request_widget */ require('./smart_interval'); +const MiniPipelineGraph = require('./mini_pipeline_graph_dropdown').default; ((global) => { var indexOf = [].indexOf || function(item) { for (var i = 0, l = this.length; i < l; i += 1) { if (i in this && this[i] === item) return i; } return -1; }; @@ -285,8 +286,8 @@ require('./smart_interval'); }; MergeRequestWidget.prototype.initMiniPipelineGraph = function() { - new gl.MiniPipelineGraph({ - container: '.js-pipeline-inline-mr-widget-graph:visible', + new MiniPipelineGraph({ + container: '.js-pipeline-inline-mr-widget-graph', }).bindEvents(); }; diff --git a/app/assets/javascripts/mini_pipeline_graph_dropdown.js b/app/assets/javascripts/mini_pipeline_graph_dropdown.js index 483c201305a..3029ec17a37 100644 --- a/app/assets/javascripts/mini_pipeline_graph_dropdown.js +++ b/app/assets/javascripts/mini_pipeline_graph_dropdown.js @@ -15,97 +15,92 @@ *
* */ -(() => { - class MiniPipelineGraph { - constructor(opts = {}) { - this.container = opts.container || ''; - this.dropdownListSelector = '.js-builds-dropdown-container'; - this.getBuildsList = this.getBuildsList.bind(this); - this.stopDropdownClickPropagation(); - } - - /** - * Adds the event listener when the dropdown is opened. - * All dropdown events are fired at the .dropdown-menu's parent element. - */ - bindEvents() { - $(document).off('shown.bs.dropdown', this.container).on('shown.bs.dropdown', this.container, this.getBuildsList); - } - - /** - * When the user right clicks or cmd/ctrl + click in the job name - * the dropdown should not be closed and the link should open in another tab, - * so we stop propagation of the click event inside the dropdown. - * - * Since this component is rendered multiple times per page we need to guarantee we only - * target the click event of this component. - */ - stopDropdownClickPropagation() { - $(document).on('click', `${this.container} .js-builds-dropdown-list a.mini-pipeline-graph-dropdown-item`, (e) => { - e.stopPropagation(); - }); - } - - /** - * For the clicked stage, renders the given data in the dropdown list. - * - * @param {HTMLElement} stageContainer - * @param {Object} data - */ - renderBuildsList(stageContainer, data) { - const dropdownContainer = stageContainer.parentElement.querySelector( - `${this.dropdownListSelector} .js-builds-dropdown-list`, - ); - - dropdownContainer.innerHTML = data; - } - - /** - * For the clicked stage, gets the list of builds. - * - * All dropdown events have a relatedTarget property, - * whose value is the toggling anchor element. - * - * @param {Object} e bootstrap dropdown event - * @return {Promise} - */ - getBuildsList(e) { - const button = e.relatedTarget; - const endpoint = button.dataset.stageEndpoint; - - return $.ajax({ - dataType: 'json', - type: 'GET', - url: endpoint, - beforeSend: () => { - this.renderBuildsList(button, ''); - this.toggleLoading(button); - }, - success: (data) => { - this.toggleLoading(button); - this.renderBuildsList(button, data.html); - }, - error: () => { - this.toggleLoading(button); - new Flash('An error occurred while fetching the builds.', 'alert'); - }, - }); - } - - /** - * Toggles the visibility of the loading icon. - * - * @param {HTMLElement} stageContainer - * @return {type} - */ - toggleLoading(stageContainer) { - stageContainer.parentElement.querySelector( - `${this.dropdownListSelector} .js-builds-dropdown-loading`, - ).classList.toggle('hidden'); - } +export default class MiniPipelineGraph { + constructor(opts = {}) { + this.container = opts.container || ''; + this.dropdownListSelector = '.js-builds-dropdown-container'; + this.getBuildsList = this.getBuildsList.bind(this); } - window.gl = window.gl || {}; - window.gl.MiniPipelineGraph = MiniPipelineGraph; -})(); + /** + * Adds the event listener when the dropdown is opened. + * All dropdown events are fired at the .dropdown-menu's parent element. + */ + bindEvents() { + $(document).off('shown.bs.dropdown', this.container).on('shown.bs.dropdown', this.container, this.getBuildsList); + } + + /** + * When the user right clicks or cmd/ctrl + click in the job name + * the dropdown should not be closed and the link should open in another tab, + * so we stop propagation of the click event inside the dropdown. + * + * Since this component is rendered multiple times per page we need to guarantee we only + * target the click event of this component. + */ + stopDropdownClickPropagation() { + document.querySelector(`${this.container} .js-builds-dropdown-list a.mini-pipeline-graph-dropdown-item`).addEventListener('click', (e) => { + e.stopPropagation(); + }); + } + + /** + * For the clicked stage, renders the given data in the dropdown list. + * + * @param {HTMLElement} stageContainer + * @param {Object} data + */ + renderBuildsList(stageContainer, data) { + const dropdownContainer = stageContainer.parentElement.querySelector( + `${this.dropdownListSelector} .js-builds-dropdown-list`, + ); + + dropdownContainer.innerHTML = data; + } + + /** + * For the clicked stage, gets the list of builds. + * + * All dropdown events have a relatedTarget property, + * whose value is the toggling anchor element. + * + * @param {Object} e bootstrap dropdown event + * @return {Promise} + */ + getBuildsList(e) { + const button = e.relatedTarget; + const endpoint = button.dataset.stageEndpoint; + + return $.ajax({ + dataType: 'json', + type: 'GET', + url: endpoint, + beforeSend: () => { + this.renderBuildsList(button, ''); + this.toggleLoading(button); + }, + success: (data) => { + this.toggleLoading(button); + this.renderBuildsList(button, data.html); + this.stopDropdownClickPropagation(); + }, + error: () => { + this.toggleLoading(button); + new Flash('An error occurred while fetching the builds.', 'alert'); + }, + }); + } + + /** + * Toggles the visibility of the loading icon. + * + * @param {HTMLElement} stageContainer + * @return {type} + */ + toggleLoading(stageContainer) { + stageContainer.parentElement.querySelector( + `${this.dropdownListSelector} .js-builds-dropdown-loading`, + ).classList.toggle('hidden'); + } +} diff --git a/changelogs/unreleased/24166-close-builds-dropdown.yml b/changelogs/unreleased/24166-close-builds-dropdown.yml new file mode 100644 index 00000000000..c57ffed6b45 --- /dev/null +++ b/changelogs/unreleased/24166-close-builds-dropdown.yml @@ -0,0 +1,4 @@ +--- +title: Prevent builds dropdown to close when the user clicks in a build +merge_request: +author: diff --git a/spec/javascripts/mini_pipeline_graph_dropdown_spec.js b/spec/javascripts/mini_pipeline_graph_dropdown_spec.js index f6b3dc87cd8..4e8637054ba 100644 --- a/spec/javascripts/mini_pipeline_graph_dropdown_spec.js +++ b/spec/javascripts/mini_pipeline_graph_dropdown_spec.js @@ -1,7 +1,7 @@ /* eslint-disable no-new */ -require('~/flash'); -require('~/mini_pipeline_graph_dropdown'); +import MiniPipelineGraph from '~/mini_pipeline_graph_dropdown'; +import '~/flash'; (() => { describe('Mini Pipeline Graph Dropdown', () => { @@ -13,7 +13,7 @@ require('~/mini_pipeline_graph_dropdown'); describe('When is initialized', () => { it('should initialize without errors when no options are given', () => { - const miniPipelineGraph = new window.gl.MiniPipelineGraph(); + const miniPipelineGraph = new MiniPipelineGraph(); expect(miniPipelineGraph.dropdownListSelector).toEqual('.js-builds-dropdown-container'); }); @@ -21,7 +21,7 @@ require('~/mini_pipeline_graph_dropdown'); it('should set the container as the given prop', () => { const container = '.foo'; - const miniPipelineGraph = new window.gl.MiniPipelineGraph({ container }); + const miniPipelineGraph = new MiniPipelineGraph({ container }); expect(miniPipelineGraph.container).toEqual(container); }); @@ -29,9 +29,9 @@ require('~/mini_pipeline_graph_dropdown'); describe('When dropdown is clicked', () => { it('should call getBuildsList', () => { - const getBuildsListSpy = spyOn(gl.MiniPipelineGraph.prototype, 'getBuildsList').and.callFake(function () {}); + const getBuildsListSpy = spyOn(MiniPipelineGraph.prototype, 'getBuildsList').and.callFake(function () {}); - new gl.MiniPipelineGraph({ container: '.js-builds-dropdown-tests' }).bindEvents(); + new MiniPipelineGraph({ container: '.js-builds-dropdown-tests' }).bindEvents(); document.querySelector('.js-builds-dropdown-button').click(); @@ -41,7 +41,7 @@ require('~/mini_pipeline_graph_dropdown'); it('should make a request to the endpoint provided in the html', () => { const ajaxSpy = spyOn($, 'ajax').and.callFake(function () {}); - new gl.MiniPipelineGraph({ container: '.js-builds-dropdown-tests' }).bindEvents(); + new MiniPipelineGraph({ container: '.js-builds-dropdown-tests' }).bindEvents(); document.querySelector('.js-builds-dropdown-button').click(); expect(ajaxSpy.calls.allArgs()[0][0].url).toEqual('foobar'); @@ -53,7 +53,7 @@ require('~/mini_pipeline_graph_dropdown'); html: '\u003cli\u003e\n\u003ca class="mini-pipeline-graph-dropdown-item" href="#"\u003e\u003cspan class="ci-status-icon ci-status-icon-failed"\u003e\u003c/span\u003e\n\u003cspan class="ci-build-text"\u003ebuild\u003c/span\u003e\n\u003c/a\u003e\u003ca class="ci-action-icon-wrapper js-ci-action-icon" href="#"\u003e\u003c/a\u003e\n\u003c/li\u003e\n', }); }); - new gl.MiniPipelineGraph({ container: '.js-builds-dropdown-tests' }).bindEvents(); + new MiniPipelineGraph({ container: '.js-builds-dropdown-tests' }).bindEvents(); document.querySelector('.js-builds-dropdown-button').click(); From 12a0d5a2bca66dc332f160f09a993fef42d356d5 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Thu, 9 Mar 2017 18:39:53 +0000 Subject: [PATCH 3/5] Use harmony syntax instead of cjs --- app/assets/javascripts/merge_request_widget.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/merge_request_widget.js b/app/assets/javascripts/merge_request_widget.js index a80ae128fa4..27d78ec776c 100644 --- a/app/assets/javascripts/merge_request_widget.js +++ b/app/assets/javascripts/merge_request_widget.js @@ -3,8 +3,8 @@ /* global notifyPermissions */ /* global merge_request_widget */ -require('./smart_interval'); -const MiniPipelineGraph = require('./mini_pipeline_graph_dropdown').default; +import './smart_interval'; +import MiniPipelineGraph from './mini_pipeline_graph_dropdown'; ((global) => { var indexOf = [].indexOf || function(item) { for (var i = 0, l = this.length; i < l; i += 1) { if (i in this && this[i] === item) return i; } return -1; }; From a7a1a60421f49c1459d46a4092008af12127994c Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Thu, 9 Mar 2017 18:51:52 +0000 Subject: [PATCH 4/5] Target all build links inside a dropdown. Use jQuery to handle event delegation. --- app/assets/javascripts/merge_request_widget.js | 2 +- app/assets/javascripts/mini_pipeline_graph_dropdown.js | 10 +++++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/app/assets/javascripts/merge_request_widget.js b/app/assets/javascripts/merge_request_widget.js index 27d78ec776c..66cc270ab4d 100644 --- a/app/assets/javascripts/merge_request_widget.js +++ b/app/assets/javascripts/merge_request_widget.js @@ -287,7 +287,7 @@ import MiniPipelineGraph from './mini_pipeline_graph_dropdown'; MergeRequestWidget.prototype.initMiniPipelineGraph = function() { new MiniPipelineGraph({ - container: '.js-pipeline-inline-mr-widget-graph', + container: '.js-pipeline-inline-mr-widget-graph:visible', }).bindEvents(); }; diff --git a/app/assets/javascripts/mini_pipeline_graph_dropdown.js b/app/assets/javascripts/mini_pipeline_graph_dropdown.js index 3029ec17a37..9c58c465001 100644 --- a/app/assets/javascripts/mini_pipeline_graph_dropdown.js +++ b/app/assets/javascripts/mini_pipeline_graph_dropdown.js @@ -40,9 +40,13 @@ export default class MiniPipelineGraph { * target the click event of this component. */ stopDropdownClickPropagation() { - document.querySelector(`${this.container} .js-builds-dropdown-list a.mini-pipeline-graph-dropdown-item`).addEventListener('click', (e) => { - e.stopPropagation(); - }); + $(document).on( + 'click', + `${this.container} .js-builds-dropdown-list a.mini-pipeline-graph-dropdown-item`, + (e) => { + e.stopPropagation(); + }, + ); } /** From 9275603df5a6f8e10e6a0a3af920c5a51bd690d6 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Thu, 9 Mar 2017 22:11:12 +0000 Subject: [PATCH 5/5] Updates test to use html tags --- spec/javascripts/mini_pipeline_graph_dropdown_spec.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/spec/javascripts/mini_pipeline_graph_dropdown_spec.js b/spec/javascripts/mini_pipeline_graph_dropdown_spec.js index 4e8637054ba..e504d41d4d4 100644 --- a/spec/javascripts/mini_pipeline_graph_dropdown_spec.js +++ b/spec/javascripts/mini_pipeline_graph_dropdown_spec.js @@ -50,7 +50,13 @@ import '~/flash'; it('should not close when user uses cmd/ctrl + click', () => { spyOn($, 'ajax').and.callFake(function (params) { params.success({ - html: '\u003cli\u003e\n\u003ca class="mini-pipeline-graph-dropdown-item" href="#"\u003e\u003cspan class="ci-status-icon ci-status-icon-failed"\u003e\u003c/span\u003e\n\u003cspan class="ci-build-text"\u003ebuild\u003c/span\u003e\n\u003c/a\u003e\u003ca class="ci-action-icon-wrapper js-ci-action-icon" href="#"\u003e\u003c/a\u003e\n\u003c/li\u003e\n', + html: `
  • + + + build + + +
    • `, }); }); new MiniPipelineGraph({ container: '.js-builds-dropdown-tests' }).bindEvents();