Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(125)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 8371017: Print Preview: Changing the structure of the html/css. (Closed)

Created:
9 years, 2 months ago by dpapad
Modified:
9 years, 1 month ago
CC:
chromium-reviews, arv (Not doing code reviews)
Visibility:
Public.

Description

Print Preview: Changing the structure of the html/css. The current structure is messy and hacky. Instead changing it to a css table layout, which makes everything much cleaner. Such a structure already exists in all the options pages. BUG=86381, 86384, 82969 TEST=UI should look almost the same (ui elements should look better aligned than before). Animations should be identical. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107784

Patch Set 1 #

Patch Set 2 : Restoring animations #

Patch Set 3 : Removing unneeded css rules. #

Total comments: 2

Patch Set 4 : Refactoring css class displaytable, removing class displaytablerow #

Total comments: 10

Patch Set 5 : Addressing comments #

Patch Set 6 : Removed css class no-padding. #

Total comments: 1

Patch Set 7 : Fixing some minor cases #

Patch Set 8 : Updating unittests #

Unified diffs Side-by-side diffs Delta from patch set Stats (+169 lines, -200 lines) Patch
M chrome/browser/resources/options/options_page.css View 1 2 3 4 1 chunk +2 lines, -4 lines 0 comments Download
M chrome/browser/resources/print_preview/color_settings.html View 1 2 3 4 1 chunk +4 lines, -4 lines 0 comments Download
M chrome/browser/resources/print_preview/color_settings.js View 1 2 3 4 5 6 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/resources/print_preview/copies_settings.html View 1 2 3 4 1 chunk +20 lines, -18 lines 0 comments Download
M chrome/browser/resources/print_preview/copies_settings.js View 1 2 3 4 5 6 1 chunk +3 lines, -7 lines 0 comments Download
M chrome/browser/resources/print_preview/header_footer_settings.html View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/resources/print_preview/header_footer_settings.js View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/resources/print_preview/layout_settings.html View 1 2 3 4 1 chunk +4 lines, -4 lines 0 comments Download
M chrome/browser/resources/print_preview/layout_settings.js View 1 2 3 4 5 6 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/resources/print_preview/margin_settings.html View 1 2 3 1 chunk +8 lines, -6 lines 0 comments Download
M chrome/browser/resources/print_preview/margin_settings.js View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/resources/print_preview/page_settings.html View 1 2 3 4 1 chunk +17 lines, -15 lines 0 comments Download
M chrome/browser/resources/print_preview/page_settings.js View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/resources/print_preview/print_preview.css View 1 2 3 4 5 6 7 chunks +34 lines, -121 lines 0 comments Download
M chrome/browser/resources/print_preview/print_preview.html View 1 2 3 4 1 chunk +6 lines, -11 lines 0 comments Download
M chrome/browser/resources/print_preview/print_preview_animations.js View 1 2 3 4 5 6 1 chunk +57 lines, -0 lines 0 comments Download
M chrome/browser/resources/webui.css View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/test/data/webui/print_preview.js View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 8 (0 generated)
dpapad
9 years, 2 months ago (2011-10-24 23:48:57 UTC) #1
James Hawkins
http://codereview.chromium.org/8371017/diff/6002/chrome/browser/resources/print_preview/print_preview.css File chrome/browser/resources/print_preview/print_preview.css (right): http://codereview.chromium.org/8371017/diff/6002/chrome/browser/resources/print_preview/print_preview.css#newcode60 chrome/browser/resources/print_preview/print_preview.css:60: .displaytable { Refactor to webui.css.
9 years, 2 months ago (2011-10-24 23:50:03 UTC) #2
dpapad
Addressed jhawkins comment and also removed class displaytablerow. http://codereview.chromium.org/8371017/diff/6002/chrome/browser/resources/print_preview/print_preview.css File chrome/browser/resources/print_preview/print_preview.css (right): http://codereview.chromium.org/8371017/diff/6002/chrome/browser/resources/print_preview/print_preview.css#newcode60 chrome/browser/resources/print_preview/print_preview.css:60: .displaytable ...
9 years, 2 months ago (2011-10-25 01:03:35 UTC) #3
James Hawkins
LGTM with nits. http://codereview.chromium.org/8371017/diff/7017/chrome/browser/resources/options/options_page.css File chrome/browser/resources/options/options_page.css (right): http://codereview.chromium.org/8371017/diff/7017/chrome/browser/resources/options/options_page.css#newcode575 chrome/browser/resources/options/options_page.css:575: * in multiple locales. Should probably ...
9 years, 2 months ago (2011-10-25 01:13:32 UTC) #4
dpapad
Addressed nits. http://codereview.chromium.org/8371017/diff/7017/chrome/browser/resources/options/options_page.css File chrome/browser/resources/options/options_page.css (right): http://codereview.chromium.org/8371017/diff/7017/chrome/browser/resources/options/options_page.css#newcode575 chrome/browser/resources/options/options_page.css:575: * in multiple locales. On 2011/10/25 01:13:32, ...
9 years, 2 months ago (2011-10-25 15:36:12 UTC) #5
Evan Stade
css class names should not reflect their appearance
9 years, 2 months ago (2011-10-25 16:47:11 UTC) #6
dpapad
On 2011/10/25 16:47:11, Evan Stade wrote: > css class names should not reflect their appearance ...
9 years, 2 months ago (2011-10-25 18:12:54 UTC) #7
Evan Stade
9 years, 1 month ago (2011-10-26 23:13:14 UTC) #8 
lgtm

http://codereview.chromium.org/8371017/diff/14001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview_animations.js (right):

http://codereview.chromium.org/8371017/diff/14001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview_animations.js:132:
div.classList = '';
I'm kind of surprised this works. Don't you want div.className = ''?

Powered by Google App Engine
This is Rietveld 408576698