yellow team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

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.