admin-config icon indicating copy to clipboard operation
admin-config copied to clipboard

#57 Fix for..in bug, add tests to guard against regressions.

Open RichardBradley opened this issue 10 years ago • 6 comments

This fixes #57, and adds tests which a) demonstrate why this is required and b) should guard against regressions.

RichardBradley avatar Dec 16 '15 10:12 RichardBradley

(This is rather ugly, but I think it is necessary. If we had "angular" in scope then a most of these could be fixed with "angular.foreach", but admin-config doesn't currently have angular as a dependency.)

RichardBradley avatar Dec 16 '15 10:12 RichardBradley

I found that using "for .. of" breaks ng-admin with "ReferenceError: Symbol is not defined" errors, so I've removed that from this PR.

RichardBradley avatar Dec 16 '15 17:12 RichardBradley

Any thoughts on this? This is necessary for me to use ng-admin at the same time as polyfills like Array.find

RichardBradley avatar Apr 04 '16 10:04 RichardBradley

Any update on this one? I have added some tests which fail before this change and pass after it.

Do you not like the "for..in" syntax? It is ugly, but I think it is necessary, as demonstrated above. If you don't want to make this change, I understand.

RichardBradley avatar Aug 25 '17 15:08 RichardBradley

Sorry about that. The PR looks fine to me expect it needs to be rebased.

djhi avatar Aug 25 '17 15:08 djhi

From my point of view, this is ready to merge.

I won't rebase it myself, as I am not confident that it would get merged were I to spend that time. It should be only a few minutes work to rebase & fix the conflicts if anyone with write permission wanted to do so.

RichardBradley avatar May 30 '18 10:05 RichardBradley