Code review comment for lp:~benji/juju-gui/bug-1088507

Revision history for this message
Gary Poster (gary) wrote :

Land with changes.

Thank you for accomplishing this difficult task.

In addition to the comments below, please add a request in process.rst
that reviewers run both prod tests and debug tests. Then, in the
Makefile, either make the default test the prod test, or make it thumb
its nose at us and tell us to run one of the other targets.

Gary

https://codereview.appspot.com/6947057/diff/1/Makefile
File Makefile (right):

https://codereview.appspot.com/6947057/diff/1/Makefile#newcode293
Makefile:293: cp -r --parents */assets
"$(PWD)/build-$(1)/juju-ui/assets/")
Looks nicer to me, thanks.

https://codereview.appspot.com/6947057/diff/1/bin/merge-files
File bin/merge-files (right):

https://codereview.appspot.com/6947057/diff/1/bin/merge-files#newcode50
bin/merge-files:50: paths.push(syspath.join(process.cwd(),
'app/models/charm.js'));
As we discussed, please either figure out why readdir is not finding
charm.js, or put an XXX and a bug, and we will come back to this.

https://codereview.appspot.com/6947057/diff/1/test/index.html
File test/index.html (right):

https://codereview.appspot.com/6947057/diff/1/test/index.html#newcode9
test/index.html:9: <script
src="/juju-ui/assets/node-event-simulate.js"></script>
You explained the horror behind these two inclusions. Please add a
comment so others can enjoy.

https://codereview.appspot.com/6947057/

« Back to merge proposal