Issue 11444044: Landing Ant's quick inspector styling

Keyboard Shortcuts

	File
u :	up to issue
m :	publish + mail comments
M :	edit review message
j / k :	jump to file after / before current file
J / K :	jump to next file with a comment after / before current file
	Side-by-side diff
i :	toggle intra-line diffs
e :	expand all comments
c :	collapse all comments
s :	toggle showing all comments
n / p :	next / previous diff chunk or comment
N / P :	next / previous comment
<Up> / <Down> :	next / previous line
<Enter> :	respond to / edit current comment
d :	mark current comment as done

	Issue
u :	up to list of issues
m :	publish + mail comments
j / k :	jump to patch after / before current patch
o / <Enter> :	open current patch in side-by-side view
i :	open current patch in unified diff view

	Issue List
j / k :	jump to issue after / before current issue
o / <Enter> :	open current issue
# :	close issue

	Comment/message editing
<Ctrl> + s or <Ctrl> + Enter :	save comment
<Esc> :	cancel edit

Issue 11444044: Landing Ant's quick inspector styling

Can't Edit
Can't Publish+Mail
Start Review

Created:
10 years, 9 months ago by jeff.pihach

Modified:
10 years, 9 months ago

Reviewers:
rharding, mp+175351

Visibility:
Public.

Description

Landing Ant's quick inspector styling https://code.launchpad.net/~hatch/juju-gui/ant-inspector-styling1/+merge/175351 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 5

Patch Set 2 : Landing Ant's quick inspector styling #

Created: 10 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Stats (+85 lines, -25 lines)			Patch
A	[revision details]	View	1 chunk	+2 lines, -0 lines	0 comments	Download
A	app/assets/images/inspector-close-icon.png	View	0 chunks	+-1 lines, --1 lines	0 comments	Download
A	app/assets/images/inspector-destroy-icon.png	View	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	app/templates/charm-pre-configuration.handlebars	View	1 chunk	+1 line, -0 lines	0 comments	Download
M	lib/views/juju-inspector.less	View	2 chunks	+83 lines, -18 lines	0 comments	Download
M	lib/views/stylesheet.less	View	4 chunks	+1 line, -9 lines	0 comments	Download

Messages

Total messages: 4

Expand All Messages | Collapse All Messages

rharding

LGTM'ing because we really want this but it needs to be revisited to be properly ...

10 years, 9 months ago (2013-07-17 18:04:46 UTC) #3

LGTM'ing because we really want this but it needs to be revisited to be properly
complete. 

We should be trying very hard to not create new colors defined in hex. We should
be trying to share values already defined in vars. Places where we duplicate
chunks of styling because we've got multiple forms of a similar 'component'
(button, widget, heading, etc) should be broken into a less variable/partial we
can reuse. All of this helps reduce the css maintenance burden.

Beyond that, there's a couple things that are not correct, such as the 4px
corner radius. We've been told by UX to make all 3px. 

Comments below for specific cases.

https://codereview.appspot.com/11444044/diff/1/lib/views/juju-inspector.less
File lib/views/juju-inspector.less (right):

https://codereview.appspot.com/11444044/diff/1/lib/views/juju-inspector.less#...
lib/views/juju-inspector.less:1: @inspector-background-color: #221E1B;
these should all be off the vars.less and not hard coded color vars. Please use
the features of less to reduce duplication.

https://codereview.appspot.com/11444044/diff/1/lib/views/juju-inspector.less#...
lib/views/juju-inspector.less:4: @border-radius: 4px;
why is this 4px when all other borders have been changed to 3px?

https://codereview.appspot.com/11444044/diff/1/lib/views/juju-inspector.less#...
lib/views/juju-inspector.less:42: height: 24px;
these icons are getting repeated values. Can we abstract them into a shared var?

https://codereview.appspot.com/11444044/diff/1/lib/views/juju-inspector.less#...
lib/views/juju-inspector.less:79: @box-shadow: 0 2px 2px -2px #777777 inset;
are buttons not already a standard style we can reuse? Why the #777777? Is this
a valid color we should be using vs something already defined?

https://codereview.appspot.com/11444044/diff/1/lib/views/juju-inspector.less#...
lib/views/juju-inspector.less:80: color: #fff;
this is defined above as @inspector-color

jeff.pihach

10 years, 9 months ago (2013-07-17 18:29:09 UTC) #4

*** Submitted:

Landing Ant's quick inspector styling

R=rharding
CC=
https://codereview.appspot.com/11444044

Expand All Messages | Collapse All Messages