VictoriaRoux / oppia

Automatically exported from code.google.com/p/oppia
Apache License 2.0
0 stars 0 forks source link

Code review request #296

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
Branch name: angular-refactoring

Link to the relevant commit(s):
1) 
https://code.google.com/p/oppia/source/detail?r=11f9f20a25cf9f1a27bcf495105777b8
5eda43d6&name=angular-refactoring
2) 
https://code.google.com/p/oppia/source/detail?r=ea0a33e59fd461cc39fe5c6ae88e68f6
fdc4243c&name=angular-refactoring
3) 
https://code.google.com/p/oppia/source/detail?r=478f76a0f7ba702b0da5e6a6b46d8a83
378fa997&name=angular-refactoring
4) 
https://code.google.com/p/oppia/source/detail?r=9f4fa8ebbde07a0b0644d608aec65d6b
81a1817b&name=angular-refactoring

Purpose of code changes on this branch:
Make a few frontend changes:
  (a) Consolidate error-handling boilerplate and request encoding (this is a long-overdue refactor).
  (b) Update controller syntax as described in the Angular docs.
  (c) Add an Object.create() polyfill for IE8. (Object.create() is used in editor/EditorServices.js.)

When reviewing my code changes, please focus on:

Mostly just a sanity-check; this was mostly mechanical. I'd also ask if the 
refactored code is an improvement over the previous code (or at least not 
worse).

There were some odd issues with karma-coverage in commit (3) but I think this 
is a bug hidden somewhere in it or its dependencies; after poking at it for 
more than an hour I decided that it was not really consequential and decided to 
let it be.

After the review, I'll merge this branch into: develop

Original issue reported on code.google.com by s...@seanlip.org on 5 Aug 2014 at 3:21

GoogleCodeExporter commented 9 years ago
No specific comments, but all look good.

Error handling refactor -- nice.
Polyfill's -- nice.
Angular controller style change -- I like it better than the previous style.
HTTP interceptor -- nice, shorter the better.

You should merge to develop asap (to minimize merge/resolve suffering).

Original comment by kash...@google.com on 5 Aug 2014 at 6:18

GoogleCodeExporter commented 9 years ago
Thanks! Will merge by tonight; running tests now. (So far the merge has no 
conflicts but I suspect a bunch of branches, like the dashboard, are going to 
have some trouble...)

Original comment by s...@seanlip.org on 5 Aug 2014 at 6:42