Adds events class and specs #1194
Conversation
el-mapache
force-pushed
the
ab-event-class
branch
3 times, most recently
from
b23cf3d
to
22e99bc
Compare
**Why**: multiple js components will need an event emitter, separate class is easier to test, more robust than initial implementation
el-mapache
force-pushed
the
ab-event-class
branch
from
22e99bc
to
350c1ed
Compare
| events = new Events(); | ||
| }); | ||
|
|
||
| it('maintains a on object of to store handlers', () => { |
There was a problem hiding this comment.
This test description is unclear to me
There was a problem hiding this comment.
Oh yeah that is super unclear! I think mangled it by deleting parts of it and trying to rewrite it...
**Why**: What was there didn't make any sense.
|
@hursey013 Updated the spec description to be clearer! |
| @@ -0,0 +1,89 @@ | |||
| const Events = require('../../../../app/assets/javascripts/app/utils/events').default; | |||
| /*eslint-disable */ | |||
| const sinon = require('sinon'); | |||
There was a problem hiding this comment.
Sinon looks cool, do you think we should start moving in this direction for javascript tests? I tend to rely on feature specs but I know that's probably not the right way if we continue to add more javascript.
There was a problem hiding this comment.
Yeah, I think we definitely want to be unit testing our javascript where possible. Self contained classes that don't have explicit dependencies on the DOM or browser APIs are great candidates for that.
|
It will! I'm going to put in a PR today that does that. |
Why: multiple js components will need an event emitter, separate
class is easier to test, more robust than initial implementation
Just a little bit of refactoring, feedback welcome!