-
Notifications
You must be signed in to change notification settings - Fork 2k
feat(chat): Modify chat module to implement johnpapa styleguide. #1122
Conversation
}); | ||
|
||
// Remove the event listener when the controller instance is destroyed | ||
$scope.$on('$destroy', function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we abstract this out into a service?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That can be a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. That should be a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a styling refactor, I thought it would be good to address this as well; if we're strictly adhering to John Papa's style-guide. Might be worthwhile to start the precedent now, with regards to SOC & moving things out into services that shouldn't be in controllers.
It's ok if you want to leave this here though. I don't feel super strong about it :)
I would like to see scope changed to $scope in the tests. But otherwise, LGTM. |
@codydaig What's the reasoning behind changing scope to $scope? Just to follow suit with the other services? If that's the case, then I'd say we should remove the "$" from the others. |
LGTM, if it helps. excited to see progress on standards |
@mleanos It's to match how they are actually used. And in the code, it's referred too as $scope, so I don't think we should be changing how we reference it in the tests. |
Yeah definitely change to |
@mleanos @ilanbiala @trainerbill I think that's the last piece holding up #874 as well is the $ in front of variables and maybe one of the controller/service names. |
Similarity of the $. |
43442b2
to
1877c4b
Compare
@ilanbiala @codydaig good to merge? |
@rhutchison @codydaig LGTM, just fix the commit message so it follows these guidelines https://github.com/meanjs/mean/blob/master/CONTRIBUTING.md#commit-message-guidelines |
1877c4b
to
5d15e64
Compare
LGTM. I took it for a test drive, and everything looks good, and is working as expected. Thanks @rhutchison |
LGTM. Thanks @rhutchison!!! |
feat(chat): Modify chat module to implement johnpapa styleguide.
I know this will further stir the pot about which guidelines the project should follow, but the intention is not to delay progress, and instead push forward.
This is not up for debate - but I will take constructive feedback.