History filtering #382 #449
Conversation
Codecov Report
@@ Coverage Diff @@
## master #449 +/- ##
==========================================
- Coverage 98.46% 98.45% -0.02%
==========================================
Files 220 220
Lines 3137 3228 +91
==========================================
+ Hits 3089 3178 +89
- Misses 48 50 +2
Continue to review full report at Codecov.
|
|
Cool! Should we add a history tab to the stores as well? |
| this.updateFilteredData(); | ||
| } | ||
|
|
||
| getAllStores(){ |
There was a problem hiding this comment.
You could get rid of this function by using the values from the CurrentStores service.
| this.updateFilteredData(); | ||
| } | ||
|
|
||
| update(){ |
There was a problem hiding this comment.
I would try use the angular digest cycle more, e.g. having a function that gets called in the template getEntries() and that function returns filtered history data. Advantage: less explicit data management code.
| Type | ||
| </button> | ||
| <md-menu-content> | ||
| <md-menu-item type="checkbox" ng-click="$ctrl.update()" ng-model="$ctrl.types.groups">{{'HISTORY.TYPES.GROUPS' | translate}}</md-menu-item> |
There was a problem hiding this comment.
It is recommended to use the translate directive over filters to reduce the watches.
<span translate="HISTORY.TYPES.GROUPS"></span>
|
I'll work on this now |
use CurrentStores and CurrentUsers as up-to-date values avoid explicit update() functions
|
TODO:
|
|
|
||
| showAllStores(bool) { | ||
| angular.forEach(this.getStores(), (store) => { | ||
| store._selected = bool; |
There was a problem hiding this comment.
I don't like this, it changes the values in CurrentStores. But everything else seems to involve much more code (e.g. merging together two lists)
There was a problem hiding this comment.
I would keep selected state in it's own object, something like:
var selectedStores = {};
// select a store
selectedStores[id] = true;
// unselect a store
delete selectedStores[id]
// check if a store is selected
selectedStores[id]There was a problem hiding this comment.
How does that go together with ng-model? E.g. here https://github.com/yunity/foodsaving-frontend/pull/449/files#diff-1491a792156b8840cb4c659ffb0de4ddR14
I'm not a huge fan of using getters/setters there, too much boilerplate.
There was a problem hiding this comment.
ng-model="selectedStores[store.id]"Maybe?
Sometimes have to use the ng-click on a checkbox instead.
There was a problem hiding this comment.
There are much worse things than boilerplate too.
There was a problem hiding this comment.
Good idea, will refactor now.
(this feature here is not urgent at all, but I wanted to get it merged before the code around it changes too much.. might need to rethink my priorities though)
|
Didn't manage to enable scrolling in the user list, but otherwise it's ready for merging. |
ToDos:
Preview:
