Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7926010:
Fix X button size of subpage for touch ui (Closed)
|
Created:
9 years, 3 months ago by bshe Modified:
9 years, 3 months ago CC:
chromium-reviews Visibility:
Public. |
DescriptionFix X button size of subpage for touch ui
BUG=none
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=102348
Patch Set 1 #
Total comments: 6
Patch Set 2 : fixes based on review #Patch Set 3 : Use pp_ifdef in main CSS file instead of creat a separate CSS file for touch UI #
Total comments: 3
Patch Set 4 : Use full rules in pp_ifdef and only one pp_ifdef for entire rules #
Total comments: 2
Patch Set 5 : Tweak comments #Messages
Total messages: 21 (0 generated)
My first bug fix
Thanks Biao, this approach looks good. I just have a few minor suggestions. http://codereview.chromium.org/7926010/diff/1/chrome/browser/resources/option... File chrome/browser/resources/options/options_page_touchui.css (right): http://codereview.chromium.org/7926010/diff/1/chrome/browser/resources/option... chrome/browser/resources/options/options_page_touchui.css:1: .close-subpage { I know it's not in all CSS files, but you should probably add a copyright header to this new file,like: /* * Copyright (c) 2011 The Chromium Authors. All rights reserved. * Use of this source code is governed by a BSD-style license that can be * found in the LICENSE file. */ http://codereview.chromium.org/7926010/diff/1/chrome/browser/resources/option... chrome/browser/resources/options/options_page_touchui.css:1: .close-subpage { I'd suggest adding a small comment above the selector like the following: /* In TOUCH_UI builds, the IDR_CLOSE_BAR resource is double-size. */ It's a little confusing that one resource comes in different sizes for different builds... http://codereview.chromium.org/7926010/diff/1/chrome/browser/resources/option... chrome/browser/resources/options/options_page_touchui.css:2: background-image: url('chrome://theme/IDR_CLOSE_BAR'); you shouldn't have to repeat this property - you're just trying to override height and width. Try removing all lines except height and width - it should behave the same without them.
Done. Thanks! http://codereview.chromium.org/7926010/diff/1/chrome/browser/resources/option... File chrome/browser/resources/options/options_page_touchui.css (right): http://codereview.chromium.org/7926010/diff/1/chrome/browser/resources/option... chrome/browser/resources/options/options_page_touchui.css:1: .close-subpage { On 2011/09/16 14:29:27, Rick Byers wrote: > I know it's not in all CSS files, but you should probably add a copyright header > to this new file,like: > > /* > * Copyright (c) 2011 The Chromium Authors. All rights reserved. > * Use of this source code is governed by a BSD-style license that can be > * found in the LICENSE file. > */ Done. http://codereview.chromium.org/7926010/diff/1/chrome/browser/resources/option... chrome/browser/resources/options/options_page_touchui.css:1: .close-subpage { On 2011/09/16 14:29:27, Rick Byers wrote: > I'd suggest adding a small comment above the selector like the following: > /* In TOUCH_UI builds, the IDR_CLOSE_BAR resource is double-size. */ > > It's a little confusing that one resource comes in different sizes for different > builds... Done. http://codereview.chromium.org/7926010/diff/1/chrome/browser/resources/option... chrome/browser/resources/options/options_page_touchui.css:2: background-image: url('chrome://theme/IDR_CLOSE_BAR'); On 2011/09/16 14:29:27, Rick Byers wrote: > you shouldn't have to repeat this property - you're just trying to override > height and width. Try removing all lines except height and width - it should > behave the same without them. Done.
LGTM Erik, can you comment on whether you're happy with this approach of customizing WebUI CSS for TOUCH_UI builds? The main alternative would be to put all the rules in one file but set an attribute on the document at run-time which we can select for (like dir=rtl today, and what Evan started doing in NTP4).
I prefer having this behind an ifdef. My main concern is that people that modify the main CSS will surely forget to update the options_page_touchui.css file. Maybe we can put the ifdef in the CSS file instead?
Thanks for suggestion! I put it behind an ifdef in options_page.css file. It works for both touchui=1 and touchui=0. On 2011/09/19 18:42:32, arv wrote: > I prefer having this behind an ifdef. My main concern is that people that modify > the main CSS will surely forget to update the options_page_touchui.css file. > > Maybe we can put the ifdef in the CSS file instead?
http://codereview.chromium.org/7926010/diff/5001/chrome/browser/resources/opt... File chrome/browser/resources/options/options_page.css (right): http://codereview.chromium.org/7926010/diff/5001/chrome/browser/resources/opt... chrome/browser/resources/options/options_page.css:66: </if> Can you use else?
http://codereview.chromium.org/7926010/diff/5001/chrome/browser/resources/opt... File chrome/browser/resources/options/options_page.css (right): http://codereview.chromium.org/7926010/diff/5001/chrome/browser/resources/opt... chrome/browser/resources/options/options_page.css:63: <if expr="pp_ifdef('touchui')"> /* In TOUCH_UI builds, the IDR_CLOSE_BAR resource is double-size. */ Cool, I didn't realize that grit processed <if>s inside of flattenhtml (I thought they only applied to the outer most resource). I like that this keeps the rules local, but I worry a bit about how this will look when we've added many such conditional styles. Perhaps a good middle-ground would be to only apply <if> at the granularity of an entire rule. Eg: <if expr="pp_ifdef('touchui')"> .close-subpage { height: 32px; width: 32px; } </if> That way each rule would stand alone and still be easily readable, but may have other rules following it that override the default behavior. To me this is a nice trade-off between keeping the common case clear while also keeping the special cases nearby. Erik, what do you think? http://codereview.chromium.org/7926010/diff/5001/chrome/browser/resources/opt... chrome/browser/resources/options/options_page.css:66: </if> On 2011/09/19 22:22:14, arv wrote: > Can you use else? I don't think Grit has any support for <else>, does it? See http://codesearch.google.com/#OAMlx_jo-ck/src/tools/grit/grit/format/html_inl...
Hi Erik.
I am wondering do you have any comment on apply <if> on the granularity of
entire rule as Rick mentioned in the previous email?
<if expr="pp_ifdef('touchui')">
.close-subpage {
height: 32px;
width: 32px;
}
</if>
Also, I found that we might be able to use <if expr="not .."> as here
http://codesearch.google.com/#OAMlx_jo-ck/src/ui/resources/ui_resources.grd&e....
So
we can have something like this in the file:
<if expr="not pp_ifdef('touchui')">
.close-subpage {
height: 16px;
width: 16px;
}
</if>
<if expr="pp_ifdef('touchui')">
.close-subpage {
height: 32px;
width: 32px;
}
</if>
Do you have any comment/suggestion? Thanks very much!
Biao
On Tue, Sep 20, 2011 at 9:18 AM, <rbyers@chromium.org> wrote:
>
> http://codereview.chromium.**org/7926010/diff/5001/chrome/**
>
browser/resources/options/**options_page.css<http://codereview.chromium.org/7926010/diff/5001/chrome/browser/resources/options/options_page.css>
> File chrome/browser/resources/**options/options_page.css (right):
>
> http://codereview.chromium.**org/7926010/diff/5001/chrome/**
>
browser/resources/options/**options_page.css#newcode63<http://codereview.chromium.org/7926010/diff/5001/chrome/browser/resources/options/options_page.css#newcode63>
> chrome/browser/resources/**options/options_page.css:63: <if
> expr="pp_ifdef('touchui')"> /* In TOUCH_UI builds, the IDR_CLOSE_BAR
> resource is double-size. */
> Cool, I didn't realize that grit processed <if>s inside of flattenhtml
> (I thought they only applied to the outer most resource).
>
> I like that this keeps the rules local, but I worry a bit about how this
> will look when we've added many such conditional styles. Perhaps a good
> middle-ground would be to only apply <if> at the granularity of an
> entire rule. Eg:
>
>
> <if expr="pp_ifdef('touchui')">
> .close-subpage {
> height: 32px;
> width: 32px;
> }
> </if>
>
> That way each rule would stand alone and still be easily readable, but
> may have other rules following it that override the default behavior.
> To me this is a nice trade-off between keeping the common case clear
> while also keeping the special cases nearby. Erik, what do you think?
>
>
> http://codereview.chromium.**org/7926010/diff/5001/chrome/**
>
browser/resources/options/**options_page.css#newcode66<http://codereview.chromium.org/7926010/diff/5001/chrome/browser/resources/options/options_page.css#newcode66>
> chrome/browser/resources/**options/options_page.css:66: </if>
>
> On 2011/09/19 22:22:14, arv wrote:
>
>> Can you use else?
>>
>
> I don't think Grit has any support for <else>, does it?
> See
> http://codesearch.google.com/#**OAMlx_jo-ck/src/tools/grit/**
> grit/format/html_inline.py&**exact_package=chromium&q=%**
>
3Cif%20file:.html&type=cs&l=28<http://codesearch.google.com/#OAMlx_jo-ck/src/tools/grit/grit/format/html_inline.py&exact_package=chromium&q=%3Cif%20file:.html&type=cs&l=28>
>
>
>
http://codereview.chromium.**org/7926010/<http://codereview.chromium.org/7926...
>
On Wed, Sep 21, 2011 at 11:30, Biao She <bshe@google.com> wrote: > Hi Erik. > I am wondering do you have any comment on apply <if> on the granularity of > entire rule as Rick mentioned in the previous email? > <if expr="pp_ifdef('touchui')"> > .close-subpage { > height: 32px; > width: 32px; > } > </if> > Also, I found that we might be able to use <if expr="not .."> as > here http://codesearch.google.com/#OAMlx_jo-ck/src/ui/resources/ui_resources.grd&exact_package=chromium&q=IDR_DEFAULT_LARGE_FAVICON&type=cs&l=31. So > we can have something like this in the file: > <if expr="not pp_ifdef('touchui')"> > .close-subpage { > height: 16px; > width: 16px; > } > </if> > <if expr="pp_ifdef('touchui')"> > .close-subpage { > height: 32px; > width: 32px; > } > </if> > Do you have any comment/suggestion? Thanks very much! I'm fine either way but I lean towards only allowing full rules in ifdefs. -- erik
Done. I use the first way to override the rules.
On 2011/09/21 19:27:10, arv wrote:
> On Wed, Sep 21, 2011 at 11:30, Biao She <mailto:bshe@google.com> wrote:
> > Hi Erik.
> > I am wondering do you have any comment on apply <if> on the granularity of
> > entire rule as Rick mentioned in the previous email?
> > <if expr="pp_ifdef('touchui')">
> > .close-subpage {
> > height: 32px;
> > width: 32px;
> > }
> > </if>
> > Also, I found that we might be able to use <if expr="not .."> as
> >
>
here http://codesearch.google.com/#OAMlx_jo-ck/src/ui/resources/ui_resources.grd&exact_package=chromium&q=IDR_DEFAULT_LARGE_FAVICON&type=cs&l=31. So
> > we can have something like this in the file:
> > <if expr="not pp_ifdef('touchui')">
> > .close-subpage {
> > height: 16px;
> > width: 16px;
> > }
> > </if>
> > <if expr="pp_ifdef('touchui')">
> > .close-subpage {
> > height: 32px;
> > width: 32px;
> > }
> > </if>
> > Do you have any comment/suggestion? Thanks very much!
>
> I'm fine either way but I lean towards only allowing full rules in ifdefs.
>
> --
> erik
Great, I'm happy with this approach too. One little tweak then I think we're done and ready to start applying this pattern in more places. http://codereview.chromium.org/7926010/diff/13001/chrome/browser/resources/op... File chrome/browser/resources/options/options_page.css (right): http://codereview.chromium.org/7926010/diff/13001/chrome/browser/resources/op... chrome/browser/resources/options/options_page.css:66: .close-subpage { /* In TOUCH_UI builds, the IDR_CLOSE_BAR resource is double-size. */ Line too long (80 column limit). I suggest moving this comment up above the <if>. You can also eliminate the other comment above ("CSS rules for touchui builds" - I think it's redundant).
I have tweaked the comments in the code. Thanks. http://codereview.chromium.org/7926010/diff/13001/chrome/browser/resources/op... File chrome/browser/resources/options/options_page.css (right): http://codereview.chromium.org/7926010/diff/13001/chrome/browser/resources/op... chrome/browser/resources/options/options_page.css:66: .close-subpage { /* In TOUCH_UI builds, the IDR_CLOSE_BAR resource is double-size. */ On 2011/09/22 13:30:10, Rick Byers wrote: > Line too long (80 column limit). > I suggest moving this comment up above the <if>. You can also eliminate the > other comment above ("CSS rules for touchui builds" - I think it's redundant). Done.
lgtm
CQ is trying tha patch. Follow status at https://chromium-status.appspot.com/cq/receiver/bshe%40chromium.org/7926010/1...
List of reviewers changed.
CQ is trying tha patch. Follow status at https://chromium-status.appspot.com/cq/bshe%40chromium.org/7926010/17001
Hey Marc, something went wrong with the commit bot here. We didn't change the CL at all after starting the commit, but after all the try jobs ran it reported the following message instead of committing. We're giving it another shot now to see if it was just intermittent. If it fails again I'll direct submit. Thanks, Rick On Thu, Sep 22, 2011 at 11:34 AM, <commit-bot@chromium.org> wrote: > List of reviewers changed. > > http://codereview.chromium.**org/7926010/<http://codereview.chromium.org/7926... >
CQ is trying tha patch. Follow status at https://chromium-status.appspot.com/cq/bshe%40chromium.org/7926010/17001
CQ is trying tha patch. Follow status at https://chromium-status.appspot.com/cq/bshe@chromium.org/7926010/17001
Change committed as 102348 |
