From 60d2a7c3557964da7425c37bb871c5131f615d5e Mon Sep 17 00:00:00 2001 From: Clement Ho Date: Wed, 7 Jun 2017 10:54:54 -0500 Subject: [PATCH] Use data attributes instead of class --- .../filtered_search_manager.js | 21 ++++++++----------- app/views/shared/issuable/_nav.html.haml | 8 +++---- features/steps/project/issues/issues.rb | 4 ++-- features/steps/project/merge_requests.rb | 4 ++-- .../filtered_search/filter_issues_spec.rb | 8 +++---- .../filter_merge_requests_spec.rb | 12 +++++------ .../filtered_search_manager_spec.js | 6 +++--- 7 files changed, 30 insertions(+), 33 deletions(-) diff --git a/app/assets/javascripts/filtered_search/filtered_search_manager.js b/app/assets/javascripts/filtered_search/filtered_search_manager.js index 215cc81f256..5a160e2080f 100644 --- a/app/assets/javascripts/filtered_search/filtered_search_manager.js +++ b/app/assets/javascripts/filtered_search/filtered_search_manager.js @@ -83,14 +83,14 @@ class FilteredSearchManager { if (this.stateFilters) { this.searchStateWrapper = this.searchState.bind(this); - this.stateFilters.querySelector('.state-opened') + this.stateFilters.querySelector('[data-state="opened"]') .addEventListener('click', this.searchStateWrapper); - this.stateFilters.querySelector('.state-closed') + this.stateFilters.querySelector('[data-state="closed"]') .addEventListener('click', this.searchStateWrapper); - this.stateFilters.querySelector('.state-all') + this.stateFilters.querySelector('[data-state="all"]') .addEventListener('click', this.searchStateWrapper); - this.mergedState = this.stateFilters.querySelector('.state-merged'); + this.mergedState = this.stateFilters.querySelector('[data-state="merged"]'); if (this.mergedState) { this.mergedState.addEventListener('click', this.searchStateWrapper); } @@ -99,11 +99,11 @@ class FilteredSearchManager { unbindStateEvents() { if (this.stateFilters) { - this.stateFilters.querySelector('.state-opened') + this.stateFilters.querySelector('[data-state="opened"]') .removeEventListener('click', this.searchStateWrapper); - this.stateFilters.querySelector('.state-closed') + this.stateFilters.querySelector('[data-state="closed"]') .removeEventListener('click', this.searchStateWrapper); - this.stateFilters.querySelector('.state-all') + this.stateFilters.querySelector('[data-state="all"]') .removeEventListener('click', this.searchStateWrapper); if (this.mergedState) { @@ -500,15 +500,12 @@ class FilteredSearchManager { searchState(e) { const target = e.currentTarget; - // remove focus outline after click target.blur(); - // return class name that has a prefix of `state-` - const stateClassName = [].find.call(target.classList, name => name.match(/(state-)(\w+)/g)); + const state = target.dataset && target.dataset.state; - if (stateClassName) { - const state = stateClassName.replace('state-', ''); + if (state) { this.search(state); } } diff --git a/app/views/shared/issuable/_nav.html.haml b/app/views/shared/issuable/_nav.html.haml index a530c0e7f5a..cf7ba52d840 100644 --- a/app/views/shared/issuable/_nav.html.haml +++ b/app/views/shared/issuable/_nav.html.haml @@ -5,20 +5,20 @@ %ul.nav-links.issues-state-filters %li{ class: active_when(params[:state] == 'opened') }> - %button.btn.btn-link.state-opened{ id: 'state-opened', title: "Filter by #{page_context_word} that are currently opened.", type: 'button' } + %button.btn.btn-link{ id: 'state-opened', title: "Filter by #{page_context_word} that are currently opened.", type: 'button', data: { state: 'opened' } } #{issuables_state_counter_text(type, :opened)} - if type == :merge_requests %li{ class: active_when(params[:state] == 'merged') }> - %button.btn.btn-link.state-merged{ id: 'state-merged', title: 'Filter by merge requests that are currently merged.', type: 'button' } + %button.btn.btn-link{ id: 'state-merged', title: 'Filter by merge requests that are currently merged.', type: 'button', data: { state: 'merged' } } #{issuables_state_counter_text(type, :merged)} - closed_title = 'Filter by merge requests that are currently closed and unmerged.' %li{ class: active_when(params[:state] == 'closed') }> - %button.btn.btn-link.state-closed{ id: 'state-closed', title: closed_title, type: 'button' } + %button.btn.btn-link{ id: 'state-closed', title: closed_title, type: 'button', data: { state: 'closed' } } #{issuables_state_counter_text(type, :closed)} %li{ class: active_when(params[:state] == 'all') }> - %button.btn.btn-link.state-all{ id: 'state-all', title: "Show all #{page_context_word}.", type: 'button' } + %button.btn.btn-link{ id: 'state-all', title: "Show all #{page_context_word}.", type: 'button', data: { state: 'all' } } #{issuables_state_counter_text(type, :all)} diff --git a/features/steps/project/issues/issues.rb b/features/steps/project/issues/issues.rb index 30db6e5d4e2..11f2b5d1d87 100644 --- a/features/steps/project/issues/issues.rb +++ b/features/steps/project/issues/issues.rb @@ -28,7 +28,7 @@ class Spinach::Features::ProjectIssues < Spinach::FeatureSteps end step 'I click link "Closed"' do - find('.issues-state-filters .state-closed span', text: 'Closed').click + find('.issues-state-filters [data-state="closed"] span', text: 'Closed').click end step 'I click button "Unsubscribe"' do @@ -44,7 +44,7 @@ class Spinach::Features::ProjectIssues < Spinach::FeatureSteps end step 'I click link "All"' do - find('.issues-state-filters .state-all span', text: 'All').click + find('.issues-state-filters [data-state="all"] span', text: 'All').click # Waits for load expect(find('.issues-state-filters > .active')).to have_content 'All' end diff --git a/features/steps/project/merge_requests.rb b/features/steps/project/merge_requests.rb index 0e70b832562..7f1e9e693af 100644 --- a/features/steps/project/merge_requests.rb +++ b/features/steps/project/merge_requests.rb @@ -26,7 +26,7 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps end step 'I click link "All"' do - find('.issues-state-filters .state-all span', text: 'All').click + find('.issues-state-filters [data-state="all"] span', text: 'All').click # Waits for load expect(find('.issues-state-filters > .active')).to have_content 'All' end @@ -36,7 +36,7 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps end step 'I click link "Closed"' do - find('.issues-state-filters .state-closed span', text: 'Closed').click + find('.issues-state-filters [data-state="closed"] span', text: 'Closed').click end step 'I should see merge request "Wiki Feature"' do diff --git a/spec/features/issues/filtered_search/filter_issues_spec.rb b/spec/features/issues/filtered_search/filter_issues_spec.rb index 36f8369c142..863f8f75cd8 100644 --- a/spec/features/issues/filtered_search/filter_issues_spec.rb +++ b/spec/features/issues/filtered_search/filter_issues_spec.rb @@ -777,17 +777,17 @@ describe 'Filter issues', js: true, feature: true do end it 'open state' do - find('.issues-state-filters .state-closed').click + find('.issues-state-filters [data-state="closed"]').click wait_for_requests - find('.issues-state-filters .state-opened').click + find('.issues-state-filters [data-state="opened"]').click wait_for_requests expect(page).to have_selector('.issues-list .issue', count: 4) end it 'closed state' do - find('.issues-state-filters .state-closed').click + find('.issues-state-filters [data-state="closed"]').click wait_for_requests expect(page).to have_selector('.issues-list .issue', count: 1) @@ -795,7 +795,7 @@ describe 'Filter issues', js: true, feature: true do end it 'all state' do - find('.issues-state-filters .state-all').click + find('.issues-state-filters [data-state="all"]').click wait_for_requests expect(page).to have_selector('.issues-list .issue', count: 5) diff --git a/spec/features/merge_requests/filter_merge_requests_spec.rb b/spec/features/merge_requests/filter_merge_requests_spec.rb index 5a13189c5bf..d086be70d69 100644 --- a/spec/features/merge_requests/filter_merge_requests_spec.rb +++ b/spec/features/merge_requests/filter_merge_requests_spec.rb @@ -40,13 +40,13 @@ describe 'Filter merge requests', feature: true do end it 'does not change when closed link is clicked' do - find('.issues-state-filters .state-closed').click + find('.issues-state-filters [data-state="closed"]').click expect_assignee_visual_tokens() end it 'does not change when all link is clicked' do - find('.issues-state-filters .state-all').click + find('.issues-state-filters [data-state="all"]').click expect_assignee_visual_tokens() end @@ -73,13 +73,13 @@ describe 'Filter merge requests', feature: true do end it 'does not change when closed link is clicked' do - find('.issues-state-filters .state-closed').click + find('.issues-state-filters [data-state="closed"]').click expect_milestone_visual_tokens() end it 'does not change when all link is clicked' do - find('.issues-state-filters .state-all').click + find('.issues-state-filters [data-state="all"]').click expect_milestone_visual_tokens() end @@ -161,13 +161,13 @@ describe 'Filter merge requests', feature: true do end it 'does not change when closed link is clicked' do - find('.issues-state-filters .state-closed').click + find('.issues-state-filters [data-state="closed"]').click expect_assignee_label_visual_tokens() end it 'does not change when all link is clicked' do - find('.issues-state-filters .state-all').click + find('.issues-state-filters [data-state="all"]').click expect_assignee_label_visual_tokens() end diff --git a/spec/javascripts/filtered_search/filtered_search_manager_spec.js b/spec/javascripts/filtered_search/filtered_search_manager_spec.js index 406b25db083..2d19b1da7b3 100644 --- a/spec/javascripts/filtered_search/filtered_search_manager_spec.js +++ b/spec/javascripts/filtered_search/filtered_search_manager_spec.js @@ -106,7 +106,6 @@ describe('Filtered Search Manager', () => { const e = { currentTarget: { blur: () => {}, - classList: [], }, }; spyOn(e.currentTarget, 'blur').and.callThrough(); @@ -119,7 +118,6 @@ describe('Filtered Search Manager', () => { const e = { currentTarget: { blur: () => {}, - classList: [], }, }; @@ -131,7 +129,9 @@ describe('Filtered Search Manager', () => { const e = { currentTarget: { blur: () => {}, - classList: ['class-name', 'state-opened'], + dataset: { + state: 'opened', + }, }, };