Issue 3250002: Add an accordian effect to NTP.

Issue 3250002: Add an accordian effect to NTP. (Closed)

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

Created:
4 years, 9 months ago by Aaron Boodman

Modified:
3 years, 11 months ago

Reviewers:
arv, Miranda Callahan

CC:
chromium-reviews, jam, darin-cc_chromium.org, stuartmorgan+watch_chromium.org, ben+cc_chromium.org, Erik does not do reviews, brettw-cc_chromium.org, pam+watch_chromium.org, jshin+watch_chromium.org, Aaron Boodman

Visibility:
Public.

Description

Add an accordian effect to NTP. BUG=53248 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=57966

Patch Set 1 #

Patch Set 2 : prereivew #

Patch Set 3 : more cleanup #

Patch Set 4 : Finishing touches #

Patch Set 5 : cleanup #

Patch Set 6 : Fix menu positioning when there is a scrollbar. #

Total comments: 10

Created: 4 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+351 lines, -139 lines)			Patch
M	base/string_util.cc	View		1 chunk	+5 lines, -1 line	1 comment	Download
M	chrome/app/generated_resources.grd	View		1 chunk	+1 line, -1 line	0 comments	Download
D	chrome/app/theme/ntp_header_background.png	View		Binary file		0 comments	Download
D	chrome/app/theme/ntp_header_background_active.png	View		Binary file		0 comments	Download
D	chrome/app/theme/ntp_header_background_hover.png	View		Binary file		0 comments	Download
M	chrome/app/theme/theme_resources.grd	View		1 chunk	+0 lines, -3 lines	0 comments	Download
M	chrome/browser/dom_ui/app_launcher_handler.cc	View		1 chunk	+4 lines, -10 lines	0 comments	Download
M	chrome/browser/dom_ui/ntp_resource_cache.cc	View		4 chunks	+16 lines, -0 lines	0 comments	Download
M	chrome/browser/renderer_host/browser_render_process_host.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/new_new_tab.css	View	1 2 3	5 chunks	+82 lines, -46 lines	4 comments	Download
M	chrome/browser/resources/new_new_tab.html	View	1 2 3	6 chunks	+19 lines, -24 lines	2 comments	Download
M	chrome/browser/resources/new_new_tab.js	View	1 2 3 4 5	13 chunks	+161 lines, -37 lines	2 comments	Download
M	chrome/browser/resources/new_tab_theme.css	View	1 2 3	3 chunks	+39 lines, -11 lines	0 comments	Download
M	chrome/browser/resources/ntp/apps.js	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	chrome/browser/resources/ntp/most_visited.css	View		1 chunk	+1 line, -1 line	1 comment	Download
M	chrome/browser/themes/browser_theme_provider.h	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	chrome/browser/themes/browser_theme_provider.cc	View		2 chunks	+7 lines, -0 lines	0 comments	Download
M	chrome/common/extensions/extension.h	View		1 chunk	+2 lines, -3 lines	0 comments	Download
M	chrome/common/extensions/extension.cc	View		1 chunk	+9 lines, -1 line	0 comments	Download

Trybot results:

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

Miranda Callahan

LGTM http://codereview.chromium.org/3250002/diff/12001/13001 File base/string_util.cc (right): http://codereview.chromium.org/3250002/diff/12001/13001#newcode1016 base/string_util.cc:1016: while (i != format_string.end() && '$' == *i) ...

4 years, 9 months ago (2010-08-30 21:17:14 UTC) #3

arv

4 years, 9 months ago (2010-08-31 21:13:58 UTC) #4

http://codereview.chromium.org/3250002/diff/12001/13010
File chrome/browser/resources/new_new_tab.css (right):

http://codereview.chromium.org/3250002/diff/12001/13010#newcode349
chrome/browser/resources/new_new_tab.css:349: margin-left: -13px;
-webkit-margin-start: -13px;
-webkit-padding-end: 4px;

http://codereview.chromium.org/3250002/diff/12001/13010#newcode368
chrome/browser/resources/new_new_tab.css:368: padding-right: 5px;
rtl

http://codereview.chromium.org/3250002/diff/12001/13010#newcode376
chrome/browser/resources/new_new_tab.css:376: right: 0;
rtl

Here you need to add another css rule that overrides the right and left

html[dir=rtl] .section > h2 > .settings {
  right: auto;
  left: 0;
}

http://codereview.chromium.org/3250002/diff/12001/13010#newcode433
chrome/browser/resources/new_new_tab.css:433: margin-left: 0;
rtl?

http://codereview.chromium.org/3250002/diff/12001/13011
File chrome/browser/resources/new_new_tab.html (right):

http://codereview.chromium.org/3250002/diff/12001/13011#newcode134
chrome/browser/resources/new_new_tab.html:134: <h2><img class="disclosure" img
src="ntp/ntp_disclosure_triangle.png"
stray img attribute

http://codereview.chromium.org/3250002/diff/12001/13011#newcode206
chrome/browser/resources/new_new_tab.html:206: updateSimpleSection('debug',
Section.DEBUG);
Can this be done earlier?

http://codereview.chromium.org/3250002/diff/12001/13012
File chrome/browser/resources/new_new_tab.js (right):

http://codereview.chromium.org/3250002/diff/12001/13012#newcode123
chrome/browser/resources/new_new_tab.js:123: // Layout the sections in a
modified accordian. The header and miniview, if
accordion

http://codereview.chromium.org/3250002/diff/12001/13012#newcode192
chrome/browser/resources/new_new_tab.js:192: // same if document.body has no
margins.
I think we had a padding or margin on the body to make room for the
attribution...

http://codereview.chromium.org/3250002/diff/12001/13015
File chrome/browser/resources/ntp/most_visited.css (right):

http://codereview.chromium.org/3250002/diff/12001/13015#newcode7
chrome/browser/resources/ntp/most_visited.css:7: margin-top: 0;
You should be able to remove this now

Expand Messages | Collapse Messages