Trying to add uiRefresh to uiCodemirror #316

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
3 participants
@ProLoser
Owner

ProLoser commented Dec 19, 2012

New uiRefresh attribute to help with rendering bugs as outlined by this code http://pastebin.com/WGsqAnEE

Problem: tests are failing. I'm not sure why and would appreciate some help.

Also, @douglasduteil you should try to fix your commented out unit tests (I will try to help).

@petebacondarwin

View changes

modules/directives/codemirror/codemirror.js
+ // Watch ui-refresh and refresh the directive
+ if (attrs.uiRefresh) {
+ scope.$watch(attrs.uiRefresh, function(){
+ setTimeout(codemirror.refresh);

This comment has been minimized.

Show comment Hide comment
@petebacondarwin

petebacondarwin Dec 19, 2012

Owner

Why are you using setTimeout rather than $timeout here?

@petebacondarwin

petebacondarwin Dec 19, 2012

Owner

Why are you using setTimeout rather than $timeout here?

This comment has been minimized.

Show comment Hide comment
@ProLoser

ProLoser Dec 19, 2012

Owner

Hmm maybe I should change that. I've been on the fence about it but mainly I figure I don't need $timeout if I'm not triggering anything with AngularJS (and yes I should probably just do $timeout(fn, 0, false); instead).

Elsewhere though, if $timeout wasn't injected and I don't need a $scope.$apply() I sort of figured it's not worth the additional overhead. I'm happy to change it though (I'm not entrenched in this).

@ProLoser

ProLoser Dec 19, 2012

Owner

Hmm maybe I should change that. I've been on the fence about it but mainly I figure I don't need $timeout if I'm not triggering anything with AngularJS (and yes I should probably just do $timeout(fn, 0, false); instead).

Elsewhere though, if $timeout wasn't injected and I don't need a $scope.$apply() I sort of figured it's not worth the additional overhead. I'm happy to change it though (I'm not entrenched in this).

This comment has been minimized.

Show comment Hide comment
@petebacondarwin

petebacondarwin Dec 19, 2012

Owner

One good reason is that in tests, $timeout is mocked out with a pseudo-synchronous version that can be manually flushed! This might help with your testing issues.

@petebacondarwin

petebacondarwin Dec 19, 2012

Owner

One good reason is that in tests, $timeout is mocked out with a pseudo-synchronous version that can be manually flushed! This might help with your testing issues.

@douglasduteil

This comment has been minimized.

Show comment Hide comment
@douglasduteil

douglasduteil Dec 20, 2012

Member
@ProLoser

This comment has been minimized.

Show comment Hide comment
@ProLoser

ProLoser Jan 30, 2013

Owner

Need to fix these unit tests @joseym again if you have some time feel free to help me out, but I'm going to try tackling this myself tonight if possible.

Owner

ProLoser commented Jan 30, 2013

Need to fix these unit tests @joseym again if you have some time feel free to help me out, but I'm going to try tackling this myself tonight if possible.

@ProLoser

This comment has been minimized.

Show comment Hide comment
@ProLoser

ProLoser Feb 9, 2013

Owner

@boneskull @petebacondarwin @ajoslin @pkozlowski-opensource if any of you guys could take a glance at this, I have all but one last test passing.

For some reason the test that is supposed to take the defaults from uiConfig is failing. I am not sure what I'm doing wrong. If I can get this test passing I can finally merge this PR into the master

Owner

ProLoser commented Feb 9, 2013

@boneskull @petebacondarwin @ajoslin @pkozlowski-opensource if any of you guys could take a glance at this, I have all but one last test passing.

For some reason the test that is supposed to take the defaults from uiConfig is failing. I am not sure what I'm doing wrong. If I can get this test passing I can finally merge this PR into the master

@ProLoser

This comment has been minimized.

Show comment Hide comment
@ProLoser

ProLoser Feb 11, 2013

Owner
Owner

ProLoser commented Feb 11, 2013

@ProLoser ProLoser closed this Feb 11, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment