Contrast and small UX fix for the search button #2708
Conversation
|
Hey @xavijam @viddo @carlostallon CR this, please. |
|
Ok I've just detected a bug here. I need to clean the search also when clicking (if search applied) |
| this.$('.js-search-input').focus(); | ||
|
|
||
| active = this.$('.Filters-inner').hasClass('search--enabled') | ||
| this.$('.Filters-inner')[ active ? 'removeClass' : 'addClass' ]('search--enabled'); |
There was a problem hiding this comment.
Would this.$('.Filters-inner').toggleClass('search--enabled')work?
There was a problem hiding this comment.
changed on the last commit as I found another thing
There was a problem hiding this comment.
- Checking current state is better done through models than the DOM, since the models act as "source of truth" and makes it easier to reason about the code.
- There is already exist a method that updates the state properly (and the view re-renders accordingly), so I would use that instead than fiddling with the visual state manually.
Thus, I would the replace the changes here with something like:
// TODO: taken from line 119, I would wrap this check in a semantic method like router.model.isSearch()
// if (this.router.model.isSearch()) {
if (this.router.model.get('q') || this.router.model.get('tag')) {
this._onCleanSearchClick()
} else {
this.$('.Filters-inner').addClass('search--enabled');
this.$('.js-search-input').focus();
}
| @@ -233,8 +233,14 @@ module.exports = cdb.core.View.extend({ | |||
|
|
|||
| _onSearchClick: function(e) { | |||
There was a problem hiding this comment.
Not sure about the readability / style on this method.
- Make variables private to function scope - Extract isSearching check to router model
|
@saleiva Found some (code) issues while reviewing it locally, so refactored it a little, and added test cases while at it (see last two commits). I'll take care of merging & releasing it with my other pending PRs. Thanks for the contributions. :) |
|
Frontend tests were OK 👍 (details) |
|
cool! |
Contrast and small UX fix for the search button
Search button should close the search state if clicked again