Commit 74c0e758 authored by Fatih Acet's avatar Fatih Acet

Merge branch 'itchy-trigger-finger-on-issues-search' into 'master'

Make issues search less finicky

## What does this MR do?

1. Tracks issues search pristine-ness, to ignore non-mutating keyups.
2. Increase debounce wait on issues search execution from 500ms to 1000ms.
3. Ensures issues search retains focus after search execution

Note: Issues search is being overhauled (https://gitlab.com/gitlab-org/gitlab-ce/issues/21747), so most (if not all) of these changes will no longer be used. But given that the overhaul has been pushed back a release (8.14?), it makes sense to do some quick fixes to improve UX in the meantime.

## Are there points in the code the reviewer needs to double check?

Will adding autofocus to the search input create unforeseen problems?

## Why was this MR needed?

- At the moment, issues search is run on any keyup, even if search terms remain the same. This is an oversight that is both a tax on servers and an annoyance to users. 

- Searches are executed pretty quickly after a gap in keyups. It's too fast according to internal and enterprise customer feedback.

- Focus is lost when a search is conducted, so you have to either tab to (any sane person would not do this, given our tab order) or reach for the mouse and select the input again. 

These are all pretty heavily complained about issues that are, to quote community users, "rage-inducing" and "major accessibility issue[s]".

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- [x] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)
- [x] API support added
- Tests
  - [x] All builds are passing
- [x] Conform by the [merge request performance guides](http://docs.gitlab.com/ce/development/merge_request_performance_guidelines.html)
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

## What are the relevant issue numbers?

https://gitlab.com/gitlab-org/gitlab-ce/issues/21503
https://gitlab.com/gitlab-org/gitlab-ce/issues/21984
https://gitlab.com/gitlab-org/gitlab-ce/issues/21597

See merge request !6735
parents 067bbd71 f1bb7ddf
...@@ -105,6 +105,7 @@ Please view this file on the master branch, on stable branches it's out of date. ...@@ -105,6 +105,7 @@ Please view this file on the master branch, on stable branches it's out of date.
- Add RTL support to markdown renderer (Ebrahim Byagowi) - Add RTL support to markdown renderer (Ebrahim Byagowi)
- Add word-wrap to issue title on issue and milestone boards (ClemMakesApps) - Add word-wrap to issue title on issue and milestone boards (ClemMakesApps)
- Fix todos page mobile viewport layout (ClemMakesApps) - Fix todos page mobile viewport layout (ClemMakesApps)
- Make issues search less finicky
- Fix inconsistent highlighting of already selected activity nav-links (ClemMakesApps) - Fix inconsistent highlighting of already selected activity nav-links (ClemMakesApps)
- Remove redundant mixins (ClemMakesApps) - Remove redundant mixins (ClemMakesApps)
- Added 'Download' button to the Snippets page (Justin DiPierro) - Added 'Download' button to the Snippets page (Justin DiPierro)
......
...@@ -15,16 +15,61 @@ ...@@ -15,16 +15,61 @@
return Issuable.labelRow = _.template('<% _.each(labels, function(label){ %> <span class="label-row btn-group" role="group" aria-label="<%- label.title %>" style="color: <%- label.text_color %>;"> <a href="#" class="btn btn-transparent has-tooltip" style="background-color: <%- label.color %>;" title="<%- label.description %>" data-container="body"> <%- label.title %> </a> <button type="button" class="btn btn-transparent label-remove js-label-filter-remove" style="background-color: <%- label.color %>;" data-label="<%- label.title %>"> <i class="fa fa-times"></i> </button> </span> <% }); %>'); return Issuable.labelRow = _.template('<% _.each(labels, function(label){ %> <span class="label-row btn-group" role="group" aria-label="<%- label.title %>" style="color: <%- label.text_color %>;"> <a href="#" class="btn btn-transparent has-tooltip" style="background-color: <%- label.color %>;" title="<%- label.description %>" data-container="body"> <%- label.title %> </a> <button type="button" class="btn btn-transparent label-remove js-label-filter-remove" style="background-color: <%- label.color %>;" data-label="<%- label.title %>"> <i class="fa fa-times"></i> </button> </span> <% }); %>');
}, },
initSearch: function() { initSearch: function() {
const $searchInput = $('#issuable_search');
Issuable.initSearchState($searchInput);
// `immediate` param set to false debounces on the `trailing` edge, lets user finish typing // `immediate` param set to false debounces on the `trailing` edge, lets user finish typing
const debouncedExecSearch = _.debounce(Issuable.executeSearch, 500, false); const debouncedExecSearch = _.debounce(Issuable.executeSearch, 1000, false);
$('#issuable_search').off('keyup').on('keyup', debouncedExecSearch); $searchInput.off('keyup').on('keyup', debouncedExecSearch);
// ensures existing filters are preserved when manually submitted // ensures existing filters are preserved when manually submitted
$('#issue_search_form').on('submit', (e) => { $('#issuable_search_form').on('submit', (e) => {
e.preventDefault(); e.preventDefault();
debouncedExecSearch(e); debouncedExecSearch(e);
}); });
},
initSearchState: function($searchInput) {
const currentSearchVal = $searchInput.val();
Issuable.searchState = {
elem: $searchInput,
current: currentSearchVal
};
Issuable.maybeFocusOnSearch();
},
accessSearchPristine: function(set) {
// store reference to previous value to prevent search on non-mutating keyup
const state = Issuable.searchState;
const currentSearchVal = state.elem.val();
if (set) {
state.current = currentSearchVal;
} else {
return state.current === currentSearchVal;
}
},
maybeFocusOnSearch: function() {
const currentSearchVal = Issuable.searchState.current;
if (currentSearchVal && currentSearchVal !== '') {
const queryLength = currentSearchVal.length;
const $searchInput = Issuable.searchState.elem;
/* The following ensures that the cursor is initially placed at
* the end of search input when focus is applied. It accounts
* for differences in browser implementations of `setSelectionRange`
* and cursor placement for elements in focus.
*/
$searchInput.focus();
if ($searchInput.setSelectionRange) {
$searchInput.setSelectionRange(queryLength, queryLength);
} else {
$searchInput.val(currentSearchVal);
}
}
}, },
executeSearch: function(e) { executeSearch: function(e) {
const $search = $('#issuable_search'); const $search = $('#issuable_search');
...@@ -32,6 +77,11 @@ ...@@ -32,6 +77,11 @@
const $searchValue = $search.val(); const $searchValue = $search.val();
const $filtersForm = $('.js-filter-form'); const $filtersForm = $('.js-filter-form');
const $input = $(`input[name='${$searchName}']`, $filtersForm); const $input = $(`input[name='${$searchName}']`, $filtersForm);
const isPristine = Issuable.accessSearchPristine();
if (isPristine) {
return;
}
if (!$input.length) { if (!$input.length) {
$filtersForm.append(`<input type='hidden' name='${$searchName}' value='${_.escape($searchValue)}'/>`); $filtersForm.append(`<input type='hidden' name='${$searchName}' value='${_.escape($searchValue)}'/>`);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment