-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Clear search filter when switching tabs in Extension Manager #7388
Conversation
@@ -210,6 +210,7 @@ define(function (require, exports, module) { | |||
models[_activeTabIndex].scrollPos = $(".modal-body", $dlg).scrollTop(); | |||
$(this).tab("show"); | |||
$(".modal-body", $dlg).scrollTop(models[_activeTabIndex].scrollPos || 0); | |||
$searchClear.click(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also going to put focus in the searchbox which may not be what we want. What you should probably do is move the code that does the actually clear from $(".search-clear").on("click")
to its own function then call it from this handler and from the $('search-clear').on('click')
handler.
function clearSearch() {
$search.val("");
views.forEach(function (view, index) {
view.filter("");
});
}
...
on("click", ".search-clear", function (e) {
clearSearch();
if (!updateSearchDisabled()) {
$search.focus();
}
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JeffryBooher Why do you think that we shouldn't put the focus back to the searchbox? I like the way it works with this fix. I guess, that I'm missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally I think that would be pretty odd behavior too -- clicking a tab isn't normally expected to move the focus away from wherever it was before. But in this case, there's really no other place for focus to go in the dialog so it almost seems like a moot point. That is, focus should be in the text field before clicking on the tab, so having it focused after clicking the tab too doesn't feel to the user like focus has moved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, the current implemention re-focuses the search box as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There aren't that much controls in this dialog who can keep the focus and where it makes sense to leave the focus on that element. Since this dialog encourages to search and filter through extensions, the most natural behavior is to put the focus in the search field. My expectation when using this dialog is, that I can see an unfiltered result in either tab when I switch. So, clearing the search filter is a good solution for this dialog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine. Still shouldn't be implemented as $(element).click() though :)
Clear search filter when switching tabs in Extension Manager
#5856.
I did simple changes in src and wanted to fix unit test, but couldn't find any tests for function _showDialog. May be they are should be written? I could try to do it.