Conversation
|
Replace #1257 .. was getting too messy |
|
@jpetitcolas All yours :) |
|
Fix #1031 |
doc/Theming.md
Outdated
| ``` | ||
| Bind the decorator to your application: | ||
| ```js | ||
| myApp.decorator('httpErrorService',require('HttpErrorDecorator')); |
There was a problem hiding this comment.
s/httpErrorService/HttpErrorService/
jpetitcolas
left a comment
jpetitcolas
left a comment
There was a problem hiding this comment.
Fine to me. I brought a few minor changes (essentially some renaming from httpErrorService to HttpErrorService). I also tested documentation, everything is good. Mergeable if tests are green.
|
Tests are green. @Kmaschta, I let you merge? |
Kmaschta
left a comment
Kmaschta
left a comment
There was a problem hiding this comment.
Great work, it will be easier for all.
A final review with few details and we're good!
| this.handle404Error(event, error); | ||
| break; | ||
| case 403: | ||
| this.handle403Error(error); |
There was a problem hiding this comment.
Please, be consistent and give the same API for all the handlers: (event, error) or (error, event).
I'm even in favor of (error, event, toState, toParams, fromState, fromParams).
It will be more customable for the next developers and the doc will be more clear too.
There was a problem hiding this comment.
if you do this I get sonar style check errors because the parameters are not used in the method (Unused function parameters should be removed javascript : UnusedFunctionArgument)
Nothing is mentionned about this in google guidelines.
Dev can override this in the decorator anyway
| 'NEXT': 'Next »', | ||
| 'DETAIL': 'Detail', | ||
| 'STATE_CHANGE_ERROR': 'State change error: {{ message }}', | ||
| 'STATE_FORBIDDEN_ERROR': 'A server 403 error occured: {{ message }}', |
There was a problem hiding this comment.
A server 403 error occured to A permission error occured?
There was a problem hiding this comment.
Well 403 error could be something else than permission, so not sure. I will let you debate with @jpetitcolas as this was is wording for the error when we started discussing this issue (#1031 (comment))
There was a problem hiding this comment.
Let use current message, as it will be useful for users to report problem to their developper. Moreover, it is easily overridable now.
3 participants
Hopefully the final chapter :)
I have modified the documentation example and using similar code our deployment