From fa2af5e0f5e290eff32f62c7ea9f935a6ad33967 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Mon, 2 Oct 2017 13:32:53 +0100 Subject: [PATCH 1/7] Flash is now a ES6 module Reduced the technical debt around our JS flash function by making it a module that is imported rather than relying on the global function. The global function still exists mainly for technical debt with how some requests are being completed, but new JS should import the module directly. Also reduces some tech debt in the file by removing the need for jQuery. Instead Flash is now 100% vanilla JS. --- app/assets/javascripts/awards_handler.js | 2 +- .../javascripts/blob/balsamiq_viewer.js | 5 +- .../blob/file_template_mediator.js | 3 +- app/assets/javascripts/blob/viewer/index.js | 2 +- .../javascripts/boards/boards_bundle.js | 2 +- .../boards/components/board_sidebar.js | 2 +- .../boards/components/modal/footer.js | 2 +- .../boards/components/sidebar/remove_issue.js | 2 +- .../create_merge_request_dropdown.js | 2 +- .../cycle_analytics/cycle_analytics_bundle.js | 1 + .../deploy_keys/components/app.vue | 2 +- .../diff_notes/components/resolve_btn.js | 2 +- .../diff_notes/services/resolve.js | 2 +- .../environments/components/environment.vue | 2 +- .../folder/environments_folder_view.vue | 2 +- .../filtered_search/dropdown_emoji.js | 7 +- .../filtered_search/dropdown_non_user.js | 7 +- .../filtered_search/dropdown_user.js | 5 +- .../filtered_search_manager.js | 3 +- .../filtered_search_visual_tokens.js | 2 +- app/assets/javascripts/flash.js | 127 +++++++++--------- app/assets/javascripts/groups/index.js | 3 +- .../integrations/integration_settings_form.js | 4 +- .../issuable_bulk_update_actions.js | 2 +- app/assets/javascripts/issue.js | 4 +- .../javascripts/issue_show/components/app.vue | 2 +- .../javascripts/jobs/job_details_mediator.js | 2 +- app/assets/javascripts/label_manager.js | 3 +- app/assets/javascripts/main.js | 11 +- .../components/diff_file_editor.js | 2 +- .../merge_conflicts/merge_conflicts_bundle.js | 2 +- app/assets/javascripts/merge_request_tabs.js | 3 +- app/assets/javascripts/milestone.js | 3 +- .../mini_pipeline_graph_dropdown.js | 2 +- .../monitoring/components/dashboard.vue | 2 +- app/assets/javascripts/notes.js | 4 +- .../notes/components/issue_comment_form.vue | 7 +- .../notes/components/issue_discussion.vue | 4 +- .../notes/components/issue_note.vue | 5 +- .../components/issue_note_awards_list.vue | 3 +- .../notes/components/issue_notes_app.vue | 2 +- .../javascripts/notes/stores/actions.js | 10 +- .../javascripts/notifications_dropdown.js | 2 +- .../pipelines/components/stage.vue | 2 +- .../javascripts/pipelines/mixins/pipelines.js | 3 +- .../pipelines/pipeline_details_bundle.js | 3 +- .../pipelines/pipeline_details_mediatior.js | 3 +- app/assets/javascripts/profile/profile.js | 2 +- .../protected_branch_edit.js | 5 +- .../protected_tags/protected_tag_edit.js | 5 +- .../repo/components/repo_commit_section.vue | 2 +- .../javascripts/repo/helpers/repo_helper.js | 3 +- .../javascripts/repo/services/repo_service.js | 6 +- app/assets/javascripts/search.js | 2 +- .../components/assignees/sidebar_assignees.js | 3 +- .../confidential_issue_sidebar.vue | 2 +- .../sidebar/lib/sidebar_move_issue.js | 2 +- .../javascripts/sidebar/sidebar_mediator.js | 3 +- app/assets/javascripts/star.js | 3 +- app/assets/javascripts/task_list.js | 3 +- .../components/mr_widget_deployment.js | 3 +- .../mr_widget_merge_when_pipeline_succeeds.js | 2 +- .../components/states/mr_widget_merged.js | 3 +- .../states/mr_widget_ready_to_merge.js | 2 +- .../components/states/mr_widget_wip.js | 2 +- .../mr_widget_options.js | 3 +- .../vue_shared/components/markdown/field.vue | 2 +- 67 files changed, 152 insertions(+), 183 deletions(-) diff --git a/app/assets/javascripts/awards_handler.js b/app/assets/javascripts/awards_handler.js index 4f01345ee3b..622764107ad 100644 --- a/app/assets/javascripts/awards_handler.js +++ b/app/assets/javascripts/awards_handler.js @@ -1,8 +1,8 @@ /* eslint-disable class-methods-use-this */ -/* global Flash */ import _ from 'underscore'; import Cookies from 'js-cookie'; import { isInIssuePage, updateTooltipTitle } from './lib/utils/common_utils'; +import Flash from './flash'; const animationEndEventString = 'animationend webkitAnimationEnd MSAnimationEnd oAnimationEnd'; const transitionEndEventString = 'transitionend webkitTransitionEnd oTransitionEnd MSTransitionEnd'; diff --git a/app/assets/javascripts/blob/balsamiq_viewer.js b/app/assets/javascripts/blob/balsamiq_viewer.js index 8641a6fdae6..062577af385 100644 --- a/app/assets/javascripts/blob/balsamiq_viewer.js +++ b/app/assets/javascripts/blob/balsamiq_viewer.js @@ -1,9 +1,8 @@ -/* global Flash */ - +import Flash from '../flash'; import BalsamiqViewer from './balsamiq/balsamiq_viewer'; function onError() { - const flash = new window.Flash('Balsamiq file could not be loaded.'); + const flash = new Flash('Balsamiq file could not be loaded.'); return flash; } diff --git a/app/assets/javascripts/blob/file_template_mediator.js b/app/assets/javascripts/blob/file_template_mediator.js index a20c6ca7a21..583e5faa506 100644 --- a/app/assets/javascripts/blob/file_template_mediator.js +++ b/app/assets/javascripts/blob/file_template_mediator.js @@ -1,6 +1,5 @@ /* eslint-disable class-methods-use-this */ -/* global Flash */ - +import Flash from '../flash'; import FileTemplateTypeSelector from './template_selectors/type_selector'; import BlobCiYamlSelector from './template_selectors/ci_yaml_selector'; import DockerfileSelector from './template_selectors/dockerfile_selector'; diff --git a/app/assets/javascripts/blob/viewer/index.js b/app/assets/javascripts/blob/viewer/index.js index e0b73f13d36..54132e8537b 100644 --- a/app/assets/javascripts/blob/viewer/index.js +++ b/app/assets/javascripts/blob/viewer/index.js @@ -1,4 +1,4 @@ -/* global Flash */ +import Flash from '../../flash'; import { handleLocationHash } from '../../lib/utils/common_utils'; export default class BlobViewer { diff --git a/app/assets/javascripts/boards/boards_bundle.js b/app/assets/javascripts/boards/boards_bundle.js index 815248f38ee..ef4093b59e3 100644 --- a/app/assets/javascripts/boards/boards_bundle.js +++ b/app/assets/javascripts/boards/boards_bundle.js @@ -1,10 +1,10 @@ /* eslint-disable one-var, quote-props, comma-dangle, space-before-function-paren */ /* global BoardService */ -/* global Flash */ import _ from 'underscore'; import Vue from 'vue'; import VueResource from 'vue-resource'; +import Flash from '../flash'; import FilteredSearchBoards from './filtered_search_boards'; import eventHub from './eventhub'; import './models/issue'; diff --git a/app/assets/javascripts/boards/components/board_sidebar.js b/app/assets/javascripts/boards/components/board_sidebar.js index 590b7be36e3..7f3afefc9cc 100644 --- a/app/assets/javascripts/boards/components/board_sidebar.js +++ b/app/assets/javascripts/boards/components/board_sidebar.js @@ -3,9 +3,9 @@ /* global MilestoneSelect */ /* global LabelsSelect */ /* global Sidebar */ -/* global Flash */ import Vue from 'vue'; +import Flash from '../../flash'; import eventHub from '../../sidebar/event_hub'; import AssigneeTitle from '../../sidebar/components/assignees/assignee_title'; import Assignees from '../../sidebar/components/assignees/assignees'; diff --git a/app/assets/javascripts/boards/components/modal/footer.js b/app/assets/javascripts/boards/components/modal/footer.js index a656f0546c0..de9e44cef35 100644 --- a/app/assets/javascripts/boards/components/modal/footer.js +++ b/app/assets/javascripts/boards/components/modal/footer.js @@ -1,7 +1,7 @@ /* eslint-disable no-new */ -/* global Flash */ import Vue from 'vue'; +import Flash from '../../../flash'; import './lists_dropdown'; const ModalStore = gl.issueBoards.ModalStore; diff --git a/app/assets/javascripts/boards/components/sidebar/remove_issue.js b/app/assets/javascripts/boards/components/sidebar/remove_issue.js index 1e623cf58b7..1ad97211934 100644 --- a/app/assets/javascripts/boards/components/sidebar/remove_issue.js +++ b/app/assets/javascripts/boards/components/sidebar/remove_issue.js @@ -1,7 +1,7 @@ /* eslint-disable no-new */ -/* global Flash */ import Vue from 'vue'; +import Flash from '../../../flash'; const Store = gl.issueBoards.BoardsStore; diff --git a/app/assets/javascripts/create_merge_request_dropdown.js b/app/assets/javascripts/create_merge_request_dropdown.js index ff2f2c81971..bf40eb3ee11 100644 --- a/app/assets/javascripts/create_merge_request_dropdown.js +++ b/app/assets/javascripts/create_merge_request_dropdown.js @@ -1,5 +1,5 @@ /* eslint-disable no-new */ -/* global Flash */ +import Flash from './flash'; import DropLab from './droplab/drop_lab'; import ISetter from './droplab/plugins/input_setter'; diff --git a/app/assets/javascripts/cycle_analytics/cycle_analytics_bundle.js b/app/assets/javascripts/cycle_analytics/cycle_analytics_bundle.js index cdf5e3c0290..d184fcfeebd 100644 --- a/app/assets/javascripts/cycle_analytics/cycle_analytics_bundle.js +++ b/app/assets/javascripts/cycle_analytics/cycle_analytics_bundle.js @@ -2,6 +2,7 @@ import Vue from 'vue'; import Cookies from 'js-cookie'; +import Flash from '../flash'; import Translate from '../vue_shared/translate'; import banner from './components/banner.vue'; import stageCodeComponent from './components/stage_code_component.vue'; diff --git a/app/assets/javascripts/deploy_keys/components/app.vue b/app/assets/javascripts/deploy_keys/components/app.vue index a663e30dfd0..54e13b79a4f 100644 --- a/app/assets/javascripts/deploy_keys/components/app.vue +++ b/app/assets/javascripts/deploy_keys/components/app.vue @@ -1,5 +1,5 @@ ', 'alert'); + + expect( + el.querySelector('.flash-text').textContent.trim(), + ).toBe(''); + }); + }); + + describe('hideFlash', () => { + let el; + + beforeEach(() => { + el = document.createElement('div'); + el.className = 'js-testing'; + }); + + it('sets transition style', () => { + hideFlash(el); + + expect( + el.style.transition, + ).toBe('opacity 0.3s'); + }); + + it('sets opacity style', () => { + hideFlash(el); + + expect( + el.style.opacity, + ).toBe('0'); + }); + + it('removes element after transitionend', () => { + document.body.appendChild(el); + + hideFlash(el); + el.dispatchEvent(new Event('transitionend')); + + expect( + document.querySelector('.js-testing'), + ).toBeNull(); + }); + + it('calls event listener callback once', () => { + spyOn(el, 'remove').and.callThrough(); + document.body.appendChild(el); + + hideFlash(el); + + el.dispatchEvent(new Event('transitionend')); + el.dispatchEvent(new Event('transitionend')); + + expect( + el.remove.calls.count(), + ).toBe(1); + }); + }); + + describe('createAction', () => { + let el; + + beforeEach(() => { + el = document.createElement('div'); + }); + + it('creates link with href', () => { + el.innerHTML = createAction({ + href: 'testing', + title: 'test', + }); + + expect( + el.querySelector('.flash-action').href, + ).toContain('testing'); + }); + + it('uses hash as href when no href is present', () => { + el.innerHTML = createAction({ + title: 'test', + }); + + expect( + el.querySelector('.flash-action').href, + ).toContain('#'); + }); + + it('adds role when no href is present', () => { + el.innerHTML = createAction({ + title: 'test', + }); + + expect( + el.querySelector('.flash-action').getAttribute('role'), + ).toBe('button'); + }); + + it('escapes the title text', () => { + el.innerHTML = createAction({ + title: '', + }); + + expect( + el.querySelector('.flash-action').textContent.trim(), + ).toBe(''); + }); + }); + + describe('createFlash', () => { + describe('no flash-container', () => { + it('does not add to the DOM', () => { + const el = flash('test'); + const flashEl = flash('testing'); + + expect( + flashEl, + ).toBeNull(); + expect( + document.querySelector('.flash-alert'), + ).toBeNull(); + }); + }); + + describe('with flash-container', () => { + beforeEach(() => { + document.body.innerHTML += ` +
+
+
+ `; + }); + + afterEach(() => { + document.querySelector('.js-content-wrapper').remove(); + }); + + it('adds flash element into container', () => { + flash('test'); + + expect( + document.querySelector('.flash-alert'), + ).not.toBeNull(); + }); + + it('adds flash into specified parent', () => { + flash( + 'test', + 'alert', + document.querySelector('.content-wrapper'), + ); + + expect( + document.querySelector('.content-wrapper .flash-alert'), + ).not.toBeNull(); + }); + + it('adds container classes when inside content-wrapper', () => { + flash('test'); + + expect( + document.querySelector('.flash-text').className, + ).toBe('flash-text container-fluid container-limited') + }); + + it('does not add container when outside of content-wrapper', () => { + document.querySelector('.content-wrapper').className = 'js-content-wrapper'; + flash('test'); + + expect( + document.querySelector('.flash-text').className, + ).toBe('flash-text') + }); + + it('removes element after clicking', () => { + flash('test', 'alert', document, null, false); + + document.querySelector('.flash-alert').click(); + + expect( + document.querySelector('.flash-alert'), + ).toBeNull(); + }); + + describe('with actionConfig', () => { + it('adds action link', () => { + flash( + 'test', + 'alert', + document, + { + title: 'test', + }, + ); + + expect( + document.querySelector('.flash-action'), + ).not.toBeNull(); + }); + + it('calls actionConfig clickHandler on click', () => { + const actionConfig = { + title: 'test', + clickHandler: jasmine.createSpy('actionConfig'), + }; + + flash( + 'test', + 'alert', + document, + actionConfig, + ); + + document.querySelector('.flash-action').click(); + + expect( + actionConfig.clickHandler, + ).toHaveBeenCalled(); + }); + }); + }); + }); +}); From 6c97107d37771522d1deec6007a791332270ba6f Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Wed, 4 Oct 2017 14:44:43 +0100 Subject: [PATCH 4/7] fixed eslint --- app/assets/javascripts/flash.js | 16 ++++++++-------- spec/javascripts/flash_spec.js | 5 ++--- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/app/assets/javascripts/flash.js b/app/assets/javascripts/flash.js index d745483c93f..8ac2b96a22d 100644 --- a/app/assets/javascripts/flash.js +++ b/app/assets/javascripts/flash.js @@ -44,14 +44,14 @@ const createFlashEl = (message, type) => ` * along with ability to provide actionConfig which can be used to show * additional action or link on banner next to message * - * @param {String} message Flash message text - * @param {String} type Type of Flash, it can be `notice` or `alert` (default) - * @param {Object} parent Reference to parent element under which Flash needs to appear - * @param {Object} actonConfig Map of config to show action on banner - * @param {String} href URL to which action config should point to (default: '#') - * @param {String} title Title of action - * @param {Function} clickHandler Method to call when action is clicked on - * @param {Boolean} fadeTransition Boolean to determine whether to fade the alert out + * @param {String} message Flash message text + * @param {String} type Type of Flash, it can be `notice` or `alert` (default) + * @param {Object} parent Reference to parent element under which Flash needs to appear + * @param {Object} actonConfig Map of config to show action on banner + * @param {String} href URL to which action config should point to (default: '#') + * @param {String} title Title of action + * @param {Function} clickHandler Method to call when action is clicked on + * @param {Boolean} fadeTransition Boolean to determine whether to fade the alert out */ const createFlash = function createFlash( message, diff --git a/spec/javascripts/flash_spec.js b/spec/javascripts/flash_spec.js index 1feb31f5c4f..66cc76ee626 100644 --- a/spec/javascripts/flash_spec.js +++ b/spec/javascripts/flash_spec.js @@ -135,7 +135,6 @@ describe('Flash', () => { describe('createFlash', () => { describe('no flash-container', () => { it('does not add to the DOM', () => { - const el = flash('test'); const flashEl = flash('testing'); expect( @@ -185,7 +184,7 @@ describe('Flash', () => { expect( document.querySelector('.flash-text').className, - ).toBe('flash-text container-fluid container-limited') + ).toBe('flash-text container-fluid container-limited'); }); it('does not add container when outside of content-wrapper', () => { @@ -194,7 +193,7 @@ describe('Flash', () => { expect( document.querySelector('.flash-text').className, - ).toBe('flash-text') + ).toBe('flash-text'); }); it('removes element after clicking', () => { From cbfc97b112849299b0aaf3b5155e278d3db17c91 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Wed, 4 Oct 2017 15:55:01 +0100 Subject: [PATCH 5/7] karma spec fixes --- app/assets/javascripts/issue_show/components/app.vue | 5 ++--- app/assets/javascripts/repo/services/repo_service.js | 5 ++--- app/assets/javascripts/sidebar/lib/sidebar_move_issue.js | 6 ++---- .../components/states/mr_widget_wip.js | 5 ++--- .../integrations/integration_settings_form_spec.js | 6 +++--- spec/javascripts/notes_spec.js | 4 ++-- 6 files changed, 13 insertions(+), 18 deletions(-) diff --git a/app/assets/javascripts/issue_show/components/app.vue b/app/assets/javascripts/issue_show/components/app.vue index 0e63c723e2f..eecb56cb185 100644 --- a/app/assets/javascripts/issue_show/components/app.vue +++ b/app/assets/javascripts/issue_show/components/app.vue @@ -1,6 +1,5 @@ '); }); + + it('adds container classes when inside content wrapper', () => { + el.innerHTML = createFlashEl('testing', 'alert', true); + + expect( + el.querySelector('.flash-text').classList.contains('container-fluid'), + ).toBeTruthy(); + expect( + el.querySelector('.flash-text').classList.contains('container-limited'), + ).toBeTruthy(); + }); }); describe('hideFlash', () => { @@ -57,6 +68,17 @@ describe('Flash', () => { ).toBe('0'); }); + it('does not set styles when fadeTransition is false', () => { + hideFlash(el, false); + + expect( + el.style.opacity, + ).toBe(''); + expect( + el.style.transition, + ).toBe(''); + }); + it('removes element after transitionend', () => { document.body.appendChild(el); @@ -192,7 +214,7 @@ describe('Flash', () => { flash('test'); expect( - document.querySelector('.flash-text').className, + document.querySelector('.flash-text').className.trim(), ).toBe('flash-text'); }); From b40bac3a3c251110ddba3f90d6d1e4419a65afe3 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Thu, 5 Oct 2017 14:42:29 +0100 Subject: [PATCH 7/7] removed global eslint for remaining files --- .../javascripts/cycle_analytics/cycle_analytics_bundle.js | 2 -- .../javascripts/issue_show/components/fields/description.vue | 1 - app/assets/javascripts/jobs/job_details_bundle.js | 2 -- .../javascripts/pipelines/components/pipelines_actions.vue | 2 -- app/assets/javascripts/repo/stores/repo_store.js | 1 - 5 files changed, 8 deletions(-) diff --git a/app/assets/javascripts/cycle_analytics/cycle_analytics_bundle.js b/app/assets/javascripts/cycle_analytics/cycle_analytics_bundle.js index d184fcfeebd..49bb6c52180 100644 --- a/app/assets/javascripts/cycle_analytics/cycle_analytics_bundle.js +++ b/app/assets/javascripts/cycle_analytics/cycle_analytics_bundle.js @@ -1,5 +1,3 @@ -/* global Flash */ - import Vue from 'vue'; import Cookies from 'js-cookie'; import Flash from '../flash'; diff --git a/app/assets/javascripts/issue_show/components/fields/description.vue b/app/assets/javascripts/issue_show/components/fields/description.vue index dc902eefc5f..0aa1b2c2e31 100644 --- a/app/assets/javascripts/issue_show/components/fields/description.vue +++ b/app/assets/javascripts/issue_show/components/fields/description.vue @@ -1,5 +1,4 @@