Replace BiG-CZ Data Catalog Filter Header With Filter Sidebar #2258
Conversation
| @@ -101,6 +101,14 @@ var MapModel = Backbone.Model.extend({ | |||
| this.set('size', updatedSize); | |||
| }, | |||
|
|
|||
| toggleSecondarySidebar: function() { | |||
| var sizeCopy = _.clone(this.get('size')), | |||
There was a problem hiding this comment.
Do we need to use _.clone here? Could we not have simply done var size = this.get('size')?
There was a problem hiding this comment.
Same astoggleSidebar above, here's our discussion on how we need to trigger a change event, and the merge oddly doesn't suffice. I'll test this out again to confirm...
There was a problem hiding this comment.
Ah I see. Very well, let's keep it. Good to know I'm hard headed as ever.
| }); | ||
|
|
||
| var SearchOption = FilterModel.extend({ | ||
| defaults: function() { |
There was a problem hiding this comment.
Since FilterModel.prototype.defaults is defined at "compile time" this doesn't need to be a dynamic function. Instead we can simply do:
defaults: _.defaults({
active: false,
}, FilterModel.prototype.defaults),For example, see https://github.com/WikiWatershed/model-my-watershed/blob/develop/src/mmw/js/src/core/modals/views.js#L90-L93
| var SearchOptions = Backbone.Collection.extend({ | ||
| model: SearchOption, | ||
| var GriddedServicesFilter = SearchOption.extend({ | ||
| defaults: function() { |
| }); | ||
|
|
||
| var DateFilter = FilterModel.extend({ | ||
| defaults: function() { |
| return filter.isActive() ? ++count : count; | ||
| }; | ||
|
|
||
| return this.reduce(incrementIfActive, 0); |
There was a problem hiding this comment.
This can be made clearer and shorter like this:
var isActive = function(x) { return x.isActive(); };
return this.filter(isActive).length;| }); | ||
|
|
||
| return this.startSearch(1); | ||
| }, | ||
|
|
||
| isSearchValid: function() { | ||
| var query = this.get('query'), | ||
| dateFilter = this.get('filters').findWhere({ id: 'date'}), |
There was a problem hiding this comment.
This can be made more generic, since each filter can have a validate() method that returns true or false:
var query = this.get('query'),
validate = function(x) { return x.validate(); },
valid = this.get('filters').map(validate);
return query && _.every(valid);For simpler filters like GriddedService which is just a checkbox, we can have its validate() always return true.
|
Clicking "Reset" runs the search again, even if it doesn't need to be run. That is, if clicking Reset multiple times, a new search is run every time, whereas it should be run only the first time. |
|
Checking the GriddedServices checkbox makes the same request as not checking it. |
|
Changing the text filter re-runs the search on only the active tab, but changing the date filter re-runs the search on all tabs. While this is fast enough (at least for now), and the tabs are independent enough, it feels inconsistent. Might be worth creating a follow up card for figuring this out. On one hand, we don't want to have stale results. On the other, triggering all the searches may be taxing. |
|
The date filter can be selected before the first search. I like that. |
|
e979ba5 makes requested code-style changes. The rest address each of the bugs you found. Thanks for finding those. |
I agree about the inconsistency, so added a commit for this. The change I made for this is simple enough to remove if we change our minds. |
|
@azavea-bot rebuild |
There was a problem hiding this comment.
+1 tested. I like the stale behavior, and think it fits. If there's feedback to the contrary, we can change it later. Good idea keeping it in a separate commit that'll be easily revertible.
Code looks really good too! Nice job on this. Having smaller commits would have made this a little easier to review, but I think we did a good job nevertheless.
Friendly reminder to squash before merging.
* Add map view options to toggle map width for the double sidebar
* Remove vestiges of "sidebar wide" mode
* Restructure data catalog models
* Previously the pieces that needed to go in the filter sidebar were
disparate attributes on the FormModel and Catalog
* Move them into a consolidated FilterCollection so we can easily...
* listen to changes on all filters at once
* count how many are active
* pipe them into view separate from the form (query) section on the main
datacatalog sidebar
* The FormModel now only takes care of query input. FilterCollection is on
the Catalog.
Catalog
+
|
+----------+attr
+
FilterCollection
+
|
+---------------+-----------------------+
| |
+ +
FilterModel ...
+
+---------+------------+extend
| |
| |
v v
DateFilter SearchOption
+
|extend
|
v
GriddedServicesFilter
* Remove old filtering views and move/adapt them for the sidebar
* Add a `stale` attr to the catalog model to keep track of whether or not a catalog needs to search when it becomes active
arottersman
force-pushed
the
arr/bigcz-filter-sidebar
branch
from
10b6bf1
to
bade7dc
Compare
Overview
Consolidate all the data catalog filters into a single sidebar. This will make it easier to add more filters in the future.
This is a big, single commit. I recommend looking at the changes in the data catalog models first; that's where the meat is (see commit message). The view changes should all be either (a) moving operations into the models (b) moving views from the form into the new sidebar views.
Behavioral differences:
Connects #2232
Notes
This is maybe a little more subclassing than we usually do. My goal with all the refactoring was to make it easy to add new filters, and have them conform to similar interfaces.
Demo
Testing Instructions
./scripts/bundle.sh. Make sure the sass compiles