Review: Approve Hi Gary,
Thanks for the clean up to the Makefile and the addition of the favicon. I hadn't missed it before but think it'll be nice to have. I took the liberty of making a card for this task assuming you just forgot. Also I couldn't find a Rietveld proposal for this work so I'm reviewing old school. Unfortunately, running 'make server' locally I do not see the favicon show up in Chromium (and FF is hopelessly broken ATM). Looking at the downloaded resources I see it was served up but it does not show in the address bar. Are you actually seeing it? I'm a bit confused by the NODE_TARGETS / EXPECTED_NODE_TARGETS / FOUND_NODE_TARGETS dance. I assume we can't just compute FOUND_NODE_TARGETS and use it directly. If that is really the case could you document exactly what is going on lest someone like the Future Me try to undo your work? The 'test -d' before 'mkdir -p' is nice but unnecessary ask 'mkdir -p' will happily do a no-op and exit with 0 if the directory already exists. I see it used in a few places. Otherwise it looks good and is easier to follow. Thanks. -- https://code.launchpad.net/~gary/juju-gui/favicon/+merge/135045 Your team Juju GUI Hackers is subscribed to branch lp:juju-gui. -- Mailing list: https://launchpad.net/~yellow Post to : [email protected] Unsubscribe : https://launchpad.net/~yellow More help : https://help.launchpad.net/ListHelp
