Events Fixes #163
Events Fixes #163
Conversation
martypowell
commented
Mar 20, 2018
martypowell
commented
- Reworked how events work by storing the state in an object
- Adjusted filter-helper service to take in arrays rather than single key, values
- Fixed broken tests
| IsOngoingVisible: model.IsOngoingVisible || true, | ||
| IsSpacesReservationVisible: model.IsSpacesReservationVisible || false, | ||
| Limit: model.limit || CONSTANTS.requestChunkSize, | ||
| EventsTypes: eventTypes, // HACK: API Needs this |
There was a problem hiding this comment.
Not going ot block, but curious why this isn't model.eventTypes || [].
There was a problem hiding this comment.
good point, when i did that, i had an alias property. let me roll that back.
|
@mxsnyder this is ready for another look
|
| }; | ||
|
|
||
| /* ** Private ** */ | ||
|
|
||
| // GOOD |
There was a problem hiding this comment.
@ascire i need to remove these.
| // GOOD | ||
| const isSameDay = (day1Date, day2Date) => day1Date && day2Date ? | ||
| $window.moment(day1Date).isSame(day2Date, 'day') : | ||
| false; |
There was a problem hiding this comment.
Beautiful refactor. Because this already spans multiple lines, I might help clarify that this is a function by separating the inputs from the expression.
const isSameDay = (day1Date, day2Date) =>
day1Date && day2Date ?
$window.moment(day1Date).isSame(day2Date, 'day') :
false;
There was a problem hiding this comment.
@ascire i don't think this code was actually modified, just moved, wasn't a case for a refactor.
There was a problem hiding this comment.
It's the exact same function as on the left, just converted to a lambda expression, which is good since it has no side effects in the first place.
There was a problem hiding this comment.
@ascire I get the update, I'm just saying that function wasn't touched as part of "Events Fixes" as described above, I think it's showing in green because it was moved or copied in.
|
@mxsnyder ready for another look, a lot has changed |
karma.conf.js
Outdated
| @@ -98,7 +98,7 @@ module.exports = function(config) { | |||
|
|
|||
| // start these browsers | |||
| // available browser launchers: https://npmjs.org/browse/keyword/karma-launcher | |||
| browsers: ['Firefox'], | |||
| browsers: ['Chrome'], | |||
There was a problem hiding this comment.
This will cause a problem for Coveralls, won't it?
There was a problem hiding this comment.
@mxsnyder yea crap, sorry forgot to roll this back when i was debugging.
|
@mxsnyder karma configured to use firefox |
