Issue 5329007: UI polish changes on cellular plan activation page....

Issue 5329007: UI polish changes on cellular plan activation page.... (Closed)

Created:
10 years ago by zel

Modified:
9 years, 6 months ago

Reviewers:
Dmitry Polukhin, trond, satorux1

CC:
chromium-reviews, arv (Not doing code reviews), davemoore+watch_chromium.org, ben+cc_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src/

Visibility:
Public.

Description

UI polish changes on cellular plan activation page. TEST=make sure chrome:mobilesetup page shows the new UI now with marketing message BUG=chromium-os:9715 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=67686

Patch Set 1 #

Patch Set 2 : '' #

Total comments: 9

Patch Set 3 : '' #

Patch Set 4 : '' #

Created: 10 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+102 lines, -20 lines)			Patch
M	chrome/app/generated_resources.grd	View		1 chunk	+3 lines, -0 lines	0 comments	Download
M	chrome/browser/chromeos/dom_ui/mobile_setup_ui.cc	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	chrome/browser/resources/mobile_setup.html	View	1 2 3	7 chunks	+76 lines, -18 lines	0 comments	Download
M	chrome/browser/resources/mobile_setup.js	View	1 2	6 chunks	+21 lines, -2 lines	0 comments	Download

Messages

Total messages: 5 (0 generated)

Expand Messages | Collapse Messages

satorux1

LGTM http://codereview.chromium.org/5329007/diff/6001/chrome/browser/resources/mobile_setup.html File chrome/browser/resources/mobile_setup.html (right): http://codereview.chromium.org/5329007/diff/6001/chrome/browser/resources/mobile_setup.html#newcode110 chrome/browser/resources/mobile_setup.html:110: } Insert a blank line between styles to ...

10 years ago (2010-11-30 05:45:52 UTC) #4

zel

10 years ago (2010-11-30 06:07:52 UTC) #5

http://codereview.chromium.org/5329007/diff/6001/chrome/browser/resources/mob...
File chrome/browser/resources/mobile_setup.html (right):

http://codereview.chromium.org/5329007/diff/6001/chrome/browser/resources/mob...
chrome/browser/resources/mobile_setup.html:110: }
On 2010/11/30 05:45:52, satorux1 wrote:
> Insert a blank line between styles to be consistent with others above?

Done.

http://codereview.chromium.org/5329007/diff/6001/chrome/browser/resources/mob...
chrome/browser/resources/mobile_setup.html:165: <td class="canvasCell"><canvas
id="canvas" width="56" height="56"></canvas></td>
On 2010/11/30 05:45:52, satorux1 wrote:
> longer than 80 chars.

Done.

http://codereview.chromium.org/5329007/diff/6001/chrome/browser/resources/mob...
chrome/browser/resources/mobile_setup.html:174: <iframe src="./test.html"
id="carrierPage" frameborder="0"></iframe>
On 2010/11/30 05:45:52, satorux1 wrote:
> Do we have test.html? Otherwise, src="" may be nicer.

Done.

http://codereview.chromium.org/5329007/diff/6001/chrome/browser/resources/mob...
File chrome/browser/resources/mobile_setup.js (right):

http://codereview.chromium.org/5329007/diff/6001/chrome/browser/resources/mob...
chrome/browser/resources/mobile_setup.js:235:
'file:///usr/share/chromeos-assets/mobile/activation.html');
On 2010/11/30 05:45:52, satorux1 wrote:
> You could define this as const at the beginning of the file.
> 
> const ACTIVATION_PAGE_URL = ...

Done.

Expand Messages | Collapse Messages