Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
5649002:
DOMUI Settings: UTH: Fix up Downloads section (Closed)
|
Created:
10 years ago by tfarina Modified:
9 years, 7 months ago CC:
chromium-reviews, arv (Not doing code reviews), ben+cc_chromium.org, ceee-reviews_chromium.org, brettw-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDOMUI Settings: UTH: Fix up Downloads section
- Downloads label, input, button should be on one line.
- Remove 'You can clear these settings...' text.
BUG=63840
TEST=visual
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=69663
Patch Set 1 : #
Total comments: 2
Patch Set 2 : label #
Total comments: 4
Patch Set 3 : fix indentation and remove style attribute #
Total comments: 2
Patch Set 4 : move style attribute to css file #Patch Set 5 : #
Total comments: 1
Messages
Total messages: 26 (0 generated)
Screenshot here: http://i.imgur.com/OK3iZ.png
http://codereview.chromium.org/5649002/diff/2001/chrome/browser/resources/opt... File chrome/browser/resources/options/advanced_options.html (right): http://codereview.chromium.org/5649002/diff/2001/chrome/browser/resources/opt... chrome/browser/resources/options/advanced_options.html:114: <span i18n-content="downloadLocationBrowseTitle"></span> This should be a label for the input.
http://codereview.chromium.org/5649002/diff/2001/chrome/browser/resources/opt... File chrome/browser/resources/options/advanced_options.html (right): http://codereview.chromium.org/5649002/diff/2001/chrome/browser/resources/opt... chrome/browser/resources/options/advanced_options.html:114: <span i18n-content="downloadLocationBrowseTitle"></span> On 2010/12/05 17:46:23, James Hawkins wrote: > This should be a label for the input. If I wrap input around a label here, it disappears.
On 2010/12/05 17:46:23, James Hawkins wrote: > http://codereview.chromium.org/5649002/diff/2001/chrome/browser/resources/opt... > File chrome/browser/resources/options/advanced_options.html (right): > > http://codereview.chromium.org/5649002/diff/2001/chrome/browser/resources/opt... > chrome/browser/resources/options/advanced_options.html:114: <span > i18n-content="downloadLocationBrowseTitle"></span> > This should be a label for the input. Added the label, but now sure if what you want. PTAL!
http://codereview.chromium.org/5649002/diff/8001/chrome/browser/resources/opt... File chrome/browser/resources/options/advanced_options.html (right): http://codereview.chromium.org/5649002/diff/8001/chrome/browser/resources/opt... chrome/browser/resources/options/advanced_options.html:113: <div> Indentation is off. http://codereview.chromium.org/5649002/diff/8001/chrome/browser/resources/opt... chrome/browser/resources/options/advanced_options.html:114: <label style="display:inline;"> You don't need this style attribute.
http://codereview.chromium.org/5649002/diff/8001/chrome/browser/resources/opt... File chrome/browser/resources/options/advanced_options.html (right): http://codereview.chromium.org/5649002/diff/8001/chrome/browser/resources/opt... chrome/browser/resources/options/advanced_options.html:114: <label style="display:inline;"> On 2010/12/06 18:58:41, James Hawkins wrote: > You don't need this style attribute. I tried without it and didn't work. :(
http://codereview.chromium.org/5649002/diff/8001/chrome/browser/resources/opt... File chrome/browser/resources/options/advanced_options.html (right): http://codereview.chromium.org/5649002/diff/8001/chrome/browser/resources/opt... chrome/browser/resources/options/advanced_options.html:114: <label style="display:inline;"> On 2010/12/06 19:01:01, tfarina wrote: > On 2010/12/06 18:58:41, James Hawkins wrote: > > You don't need this style attribute. > I tried without it and didn't work. :( Works for me.
On 2010/12/06 19:07:35, James Hawkins wrote: > http://codereview.chromium.org/5649002/diff/8001/chrome/browser/resources/opt... > File chrome/browser/resources/options/advanced_options.html (right): > > http://codereview.chromium.org/5649002/diff/8001/chrome/browser/resources/opt... > chrome/browser/resources/options/advanced_options.html:114: <label > style="display:inline;"> > On 2010/12/06 19:01:01, tfarina wrote: > > On 2010/12/06 18:58:41, James Hawkins wrote: > > > You don't need this style attribute. > > I tried without it and didn't work. :( > > Works for me. OK, I might be wrong (or confused). Please take a look at patch set 3.
http://codereview.chromium.org/5649002/diff/14001/chrome/browser/resources/op... File chrome/browser/resources/options/advanced_options.html (right): http://codereview.chromium.org/5649002/diff/14001/chrome/browser/resources/op... chrome/browser/resources/options/advanced_options.html:117: </label> Doesn't this put the button on a new line?
http://codereview.chromium.org/5649002/diff/14001/chrome/browser/resources/op... File chrome/browser/resources/options/advanced_options.html (right): http://codereview.chromium.org/5649002/diff/14001/chrome/browser/resources/op... chrome/browser/resources/options/advanced_options.html:117: </label> On 2010/12/06 19:18:22, James Hawkins wrote: > Doesn't this put the button on a new line? Hum, yeah, hence the style attribute I've used. Well, I won't be able to provide a tested patch/fix for this until the weekend. Using the style attribute worked for me, so this will have to wait until the weekend. Sorry.
On 2010/12/06 19:26:03, tfarina wrote: > http://codereview.chromium.org/5649002/diff/14001/chrome/browser/resources/op... > File chrome/browser/resources/options/advanced_options.html (right): > > http://codereview.chromium.org/5649002/diff/14001/chrome/browser/resources/op... > chrome/browser/resources/options/advanced_options.html:117: </label> > On 2010/12/06 19:18:22, James Hawkins wrote: > > Doesn't this put the button on a new line? > > Hum, yeah, hence the style attribute I've used. > > Well, I won't be able to provide a tested patch/fix for this until the weekend. > Using the style attribute worked for me, so this will have to wait until the > weekend. Sorry. Using the label element correctly will alleviate this problem.
On 2010/12/06 19:27:10, James Hawkins wrote: > On 2010/12/06 19:26:03, tfarina wrote: > > > http://codereview.chromium.org/5649002/diff/14001/chrome/browser/resources/op... > > File chrome/browser/resources/options/advanced_options.html (right): > > > > > http://codereview.chromium.org/5649002/diff/14001/chrome/browser/resources/op... > > chrome/browser/resources/options/advanced_options.html:117: </label> > > On 2010/12/06 19:18:22, James Hawkins wrote: > Using the label element correctly will alleviate this problem. Since I haven't tested the patch set 3, I can't say it's right. But it shouldn't be wrong either. As cookies_view.html +6 does exactly the same thing, except that it doesn't set the size of the input.
On 2010/12/06 19:34:59, tfarina wrote: > On 2010/12/06 19:27:10, James Hawkins wrote: > > On 2010/12/06 19:26:03, tfarina wrote: > > > > > > http://codereview.chromium.org/5649002/diff/14001/chrome/browser/resources/op... > > > File chrome/browser/resources/options/advanced_options.html (right): > > > > > > > > > http://codereview.chromium.org/5649002/diff/14001/chrome/browser/resources/op... > > > chrome/browser/resources/options/advanced_options.html:117: </label> > > > On 2010/12/06 19:18:22, James Hawkins wrote: > > Using the label element correctly will alleviate this problem. > > Since I haven't tested the patch set 3, I can't say it's right. But it shouldn't > be wrong either. As cookies_view.html +6 does exactly the same thing, except > that it doesn't set the size of the input. I patched in patch set 3, and the browse button is on a new line.
Thiago, I've uploaded a CL that contains the way I would recommend doing this as: http://codereview.chromium.org/5959001/ You can either incorporate the changes here or give me a code review. thx.
On 2010/12/17 06:39:18, csilv wrote: > Thiago, > > I've uploaded a CL that contains the way I would recommend doing this as: > http://codereview.chromium.org/5959001/ > > You can either incorporate the changes here or give me a code review. thx. But that was exactly what I did on patchset 2, except that I put the style attribute on the label not on the css file, and James said that I don't need the style attribute. http://codereview.chromium.org/5649002/diff/8001/chrome/browser/resources/opt...
On 2010/12/17 11:16:14, tfarina wrote: > But that was exactly what I did on patchset 2, except that I put the style > attribute on the label not on the css file, and James said that I don't need the > style attribute. Yes, James was not correct in that comment. The options_page.css file defines label as a block element for the option pages, that's why it's necessary to redefine it as inline for this case. We also want to avoid using style tags if possible, thus using the css file instead.
On 2010/12/17 18:01:48, csilv wrote: > On 2010/12/17 11:16:14, tfarina wrote: > > But that was exactly what I did on patchset 2, except that I put the style > > attribute on the label not on the css file, and James said that I don't need > the > > style attribute. > > Yes, James was not correct in that comment. The options_page.css file defines > label as a block element for the option pages, that's why it's necessary to > redefine it as inline for this case. We also want to avoid using style tags if > possible, thus using the css file instead. Chris, please, take another look. Moved the style attribute to the css file.
On 2010/12/17 22:46:16, tfarina wrote: > On 2010/12/17 18:01:48, csilv wrote: > > On 2010/12/17 11:16:14, tfarina wrote: > > > But that was exactly what I did on patchset 2, except that I put the style > > > attribute on the label not on the css file, and James said that I don't need > > the > > > style attribute. > > > > Yes, James was not correct in that comment. The options_page.css file defines > > label as a block element for the option pages, that's why it's necessary to > > redefine it as inline for this case. We also want to avoid using style tags > if > > possible, thus using the css file instead. > > Chris, please, take another look. Moved the style attribute to the css file. Check out http://codereview.chromium.org/6046001/ which fixes the problem you are seeing with using labels correctly here. This will obviate the need to use 'display: inline' style.
On 2010/12/18 00:16:09, James Hawkins wrote: > On 2010/12/17 22:46:16, tfarina wrote: > > On 2010/12/17 18:01:48, csilv wrote: > > > On 2010/12/17 11:16:14, tfarina wrote: > > > > But that was exactly what I did on patchset 2, except that I put the style > > > > attribute on the label not on the css file, and James said that I don't > need > > > the > > > > style attribute. > > > > > > Yes, James was not correct in that comment. The options_page.css file > defines > > > label as a block element for the option pages, that's why it's necessary to > > > redefine it as inline for this case. We also want to avoid using style tags > > if > > > possible, thus using the css file instead. > > > > Chris, please, take another look. Moved the style attribute to the css file. > > Check out http://codereview.chromium.org/6046001/ which fixes the problem you > are seeing with using labels correctly here. This will obviate the need to use > 'display: inline' style. I've patched this in, and confirmed that with it the 'display: inline' is not necessary.
On 2010/12/18 00:22:46, tfarina wrote: > On 2010/12/18 00:16:09, James Hawkins wrote: > > On 2010/12/17 22:46:16, tfarina wrote: > > > On 2010/12/17 18:01:48, csilv wrote: > > > > On 2010/12/17 11:16:14, tfarina wrote: > > > > > But that was exactly what I did on patchset 2, except that I put the > style > > > > > attribute on the label not on the css file, and James said that I don't > > need > > > > the > > > > > style attribute. > > > > > > > > Yes, James was not correct in that comment. The options_page.css file > > defines > > > > label as a block element for the option pages, that's why it's necessary > to > > > > redefine it as inline for this case. We also want to avoid using style > tags > > > if > > > > possible, thus using the css file instead. > > > > > > Chris, please, take another look. Moved the style attribute to the css file. > > > > Check out http://codereview.chromium.org/6046001/ which fixes the problem you > > are seeing with using labels correctly here. This will obviate the need to > use > > 'display: inline' style. > > I've patched this in, and confirmed that with it the 'display: inline' is not > necessary. I've committed that CL now. Can you update this CL and I'll take another look?
On 2010/12/18 01:11:36, James Hawkins wrote: > > I've committed that CL now. Can you update this CL and I'll take another look? Done, PTAL.
LGTM
http://codereview.chromium.org/5649002/diff/34001/chrome/browser/resources/op... File chrome/browser/resources/options/advanced_options.html (right): http://codereview.chromium.org/5649002/diff/34001/chrome/browser/resources/op... chrome/browser/resources/options/advanced_options.html:119: <label> <label i18n-content="..."><input></label>
On 2010/12/18 18:23:20, James Hawkins wrote: > http://codereview.chromium.org/5649002/diff/34001/chrome/browser/resources/op... > File chrome/browser/resources/options/advanced_options.html (right): > > http://codereview.chromium.org/5649002/diff/34001/chrome/browser/resources/op... > chrome/browser/resources/options/advanced_options.html:119: <label> > <label i18n-content="..."><input></label> James, that doesn't work.
On 2010/12/18 02:06:27, csilv wrote: > LGTM OK to commit as is?
On 2010/12/18 23:35:06, tfarina wrote: > On 2010/12/18 02:06:27, csilv wrote: > > LGTM > > OK to commit as is? LGTM. I'll take a look at the issue I brought up locally. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
