Hi Brad. Thanks for this branch. It is cool to have the 3d buttons done in CSS.
I have a number of suggestions for you to consider, but in general I automatically approve it once you consider them. Gary https://codereview.appspot.com/6811062/diff/5001/bin/lint-yuidoc File bin/lint-yuidoc (right): https://codereview.appspot.com/6811062/diff/5001/bin/lint-yuidoc#newcode90 bin/lint-yuidoc:90: dirs[:] = remove('assets', dirs) I'd remove the function call and do it locally, once you remove the other use of the function below. (I'd also switch to "try: dirs.remove('assets') except ValueError: pass" but that's just me). If you like it, do it. https://codereview.appspot.com/6811062/diff/5001/bin/lint-yuidoc#newcode93 bin/lint-yuidoc:93: files = filter(lambda x: x.endswith('.js'), files) Without much excitement, I suggest you remove the "remove" function and consolidate this into one pass through the names. I'd lean towards the list comprehension, so I'd replace lines 92 and 93 with files = [i for i in files if i.endswith('.js') and i != 'templates.js'] As you wish. https://codereview.appspot.com/6811062/diff/5001/lib/views/stylesheet.less File lib/views/stylesheet.less (right): https://codereview.appspot.com/6811062/diff/5001/lib/views/stylesheet.less#newcode743 lib/views/stylesheet.less:743: .button-colors(@top-color, @bottom-color, @shadow-color, @v-pos) { What do you think about defining this function at the top level, and thus making it available generally? I think that's what we will want. https://codereview.appspot.com/6811062/diff/5001/lib/views/stylesheet.less#newcode757 lib/views/stylesheet.less:757: box-shadow: 0 (@v-pos + 3) @blur -@blur @shadow-color inset, 2px 0 3px -2px @shadow-color inset, -2px 0 3px -2px @shadow-color inset; I *thought* you could use "box-shadow" in LESS and it would populate the other two proprietary versions. Could you double check and simplify if I am right? If I'm wrong, please let me know also. :-) https://codereview.appspot.com/6811062/diff/5001/lib/views/stylesheet.less#newcode789 lib/views/stylesheet.less:789: .button-colors(@charm-panel-cancel-button-color, @charm-panel-cancel-button-color, @charm-panel-cancel-button-shadow, 1px); We generally are using four space indents in this file. https://codereview.appspot.com/6811062/ -- https://code.launchpad.net/~bac/juju-gui/css-buttons/+merge/132365 Your team Juju GUI Hackers is requested to review the proposed merge of lp:~bac/juju-gui/css-buttons into 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
