Closed kanhirun closed 8 years ago
@bsouthga Ship is ready for sail. Ready for review.
@kanhirun this looks great! thanks for adding code coverage + a few more tests. 👏
There is only one issue I see to take a look at before merging this in -- when I use this branch with our current app, it I get the
angular-zendesk-widget.js:15 Uncaught Error: Missing accountUrl. Please set in app config via ZendeskWidgetProvider
Error, as it looks like the new order of the concatenated files causes the run statement to occur before the config registration.
Maybe take a look at that? It also might make sense to add a real index.html
test file with an example of injecting the plugin to a fake angular module.
@bsouthga I'm not sure what the proper naming is for angular files. Could I request for a review on 1bb64e1a?
@kanhirun the filenames look good to me, don't yet have a nailed down standard.
@bsouthga I made changes based on your feedback and tested it end-to-end with the angular-seed app. Thanks again for bringing this to my attention.
The issue wasn't just that the run statement occurred before config, but that both zendeskWidgetSettings
and ZendeskWidgetProvider
share settings
in common, and it needs to remain shared even when the pieces separate into their own files—and not with their own copy of settings
. The task became, how can these two components be separated into their own files while still sharing the same data?
Ultimately, I decided to just merge the two back into the same file. (After all, my testing requirement was just to keep the run statement by itself.)
If you want to take a different approach, I'll lean on your expertise here.
Here are some other solutions:
Awesome! looks good to me -- I think keeping the two in the same file is fine for now. If we did want to move them to separate files, we could try injecting the settings into the provider like this:
angular.module('zendeskWidget')
.provider('ZendeskWidget', ['zendeskWidgetSettings', function(settings) {
//... do stuff
});
Overall great work 👍 -- appreciate your thoroughness and desire for implementing refactoring best practices!
@bsouthga Great! I'll leave it to you to press the big ✅ button then.
EDIT: To my understanding, zendeskWidgetSettings
can't exist as a dependency because angular loads providers first before loading its services (i.e. Value, Factory). Indeed if you tried doing so, you'll get Unknown provider: zendeskWidgetSettingsProvider
.
Your totally right, my mistake -- I think keeping the two in the same file is fine for now. Merged!
Summary
zendeskWidgetSettings
, andZendeskWidgetProvider
(4d95fccf6ba35f453fd7daaccceff804ba7e540a) (See Figure 1 below)integration/
andunit/
(see Figure 2 below)Figure 1. Unit Test Coverage Report
Missing 6.25% is due to Self-Executing Anonymous Function [LOC]
Figure 2. The
test/
is reorganized to expose two distinct layers