Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1583293004:
Changes to Autofill settings page to ensure Credit Card list is always at least partially visible o… (Closed)
|
Created:
4 years, 11 months ago by tmartino Modified:
4 years, 10 months ago CC:
arv+watch_chromium.org, bondd+autofillwatch_chromium.org, chromium-reviews, dbeam+watch-options_chromium.org, estade+watch_chromium.org, jdonnelly+autofillwatch_chromium.org, michaelpg+watch-options_chromium.org, rouslan+autofill_chromium.org, vabr+watchlistautofill_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChanges to Autofill settings page to ensure Credit Card list is always at least partially visible on small screens (tested safe to 440px).
BUG=575804
Committed: https://crrev.com/4325576e2971f15958ebae063446b72489679dc1
Cr-Commit-Position: refs/heads/master@{#372154}
Patch Set 1 #Patch Set 2 : Spacing tweak #
Total comments: 6
Patch Set 3 : Addressing dbeam feedback #
Total comments: 2
Patch Set 4 : Addressing second round of feedback #
Total comments: 4
Patch Set 5 : Removing superfluous extra-margin class #
Messages
Total messages: 20 (5 generated)
tmartino@chromium.org changed reviewers: + mathp@chromium.org
A nice cleanup! lgtm but not an expert in webui https://codereview.chromium.org/1583293004/diff/20001/chrome/browser/resource... File chrome/browser/resources/options/autofill_options.html (right): https://codereview.chromium.org/1583293004/diff/20001/chrome/browser/resource... chrome/browser/resources/options/autofill_options.html:24: <div class="spacer-div"></div> I notice we have class spacer-div, but I can't see where it's defined... Can we reuse or change it?
tmartino@chromium.org changed reviewers: + dbeam@chromium.org
Hi Dan, Adding you for OWNERS review on this one. https://codereview.chromium.org/1583293004/diff/20001/chrome/browser/resource... File chrome/browser/resources/options/autofill_options.html (right): https://codereview.chromium.org/1583293004/diff/20001/chrome/browser/resource... chrome/browser/resources/options/autofill_options.html:24: <div class="spacer-div"></div> On 2016/01/18 at 20:07:36, Mathieu Perreault wrote: > I notice we have class spacer-div, but I can't see where it's defined... Can we reuse or change it? I tracked it down to somewhere murky in the WebUI base definitions, so changing it is probably a no-go. I'm assuming you're talking about de-duping with the vertical-space class I defined in the CSS? spacer-div is used for filling out horizontal space only, and it does so by altering layout. My vertical-space class is just inserting 2em of empty vertical space.
Friendly ping dbeam@
can i get before and after screenshots? https://codereview.chromium.org/1583293004/diff/20001/chrome/browser/resource... File chrome/browser/resources/options/autofill_options.css (right): https://codereview.chromium.org/1583293004/diff/20001/chrome/browser/resource... chrome/browser/resources/options/autofill_options.css:10: height: 192px; /* min-height for lists in webui */ 2 spaces before comments, try to make full[er] sentence and use period at the end https://codereview.chromium.org/1583293004/diff/20001/chrome/browser/resource... chrome/browser/resources/options/autofill_options.css:11: margin-bottom: 1em; unless you're copying something, you probably ought to use px instead (fonts differ per-platform) https://codereview.chromium.org/1583293004/diff/20001/chrome/browser/resource... chrome/browser/resources/options/autofill_options.css:42: float: right; RTL https://codereview.chromium.org/1583293004/diff/20001/chrome/browser/resource... chrome/browser/resources/options/autofill_options.css:44: margin-right: 0; RTL
I made the RTL fix (switched to flex instead of float), changed the em values to px, and improved the comment language. Here are the screenshots you requested: https://docs.google.com/a/chromium.org/drawings/d/1JeKppuCXN0rjHWxCfbF72rw10h...
Friendly ping :)
i'm a little skeptical that the buttons next to titles were UI reviewed, but because this is semi-buried UI I'm willing to wait for somebody to notice code looks pretty good https://codereview.chromium.org/1583293004/diff/40001/chrome/browser/resource... File chrome/browser/resources/options/autofill_options.css (right): https://codereview.chromium.org/1583293004/diff/40001/chrome/browser/resource... chrome/browser/resources/options/autofill_options.css:41: font-size: small; what is this doing? do you mean font-size: inherit? you should probably use a % or em if you need to scale it otherwise https://codereview.chromium.org/1583293004/diff/40001/chrome/browser/resource... chrome/browser/resources/options/autofill_options.css:63: } can you just put a margin-bottom on an existing element rather than creating a class="vertical-space" one?
On 2016/01/27 at 02:22:27, dbeam wrote: > i'm a little skeptical that the buttons next to titles were UI reviewed, but because this is semi-buried UI I'm willing to wait for somebody to notice > > code looks pretty good > > https://codereview.chromium.org/1583293004/diff/40001/chrome/browser/resource... > File chrome/browser/resources/options/autofill_options.css (right): > > https://codereview.chromium.org/1583293004/diff/40001/chrome/browser/resource... > chrome/browser/resources/options/autofill_options.css:41: font-size: small; > what is this doing? do you mean font-size: inherit? you should probably use a % or em if you need to scale it otherwise > > https://codereview.chromium.org/1583293004/diff/40001/chrome/browser/resource... > chrome/browser/resources/options/autofill_options.css:63: } > can you just put a margin-bottom on an existing element rather than creating a class="vertical-space" one? Solid suggestions, thanks.
https://codereview.chromium.org/1583293004/diff/60001/chrome/browser/resource... File chrome/browser/resources/options/autofill_options.css (right): https://codereview.chromium.org/1583293004/diff/60001/chrome/browser/resource... chrome/browser/resources/options/autofill_options.css:10: height: 192px; /* This is the min-height for lists from WebUI.*/ i don't understand what this comment adds https://codereview.chromium.org/1583293004/diff/60001/chrome/browser/resource... chrome/browser/resources/options/autofill_options.css:60: #autofill-options .extra-margin { could this just be .settings-list + .autofill-section-header { margin-top: 20px; } and we nuke the extra-margin class?
https://codereview.chromium.org/1583293004/diff/60001/chrome/browser/resource... File chrome/browser/resources/options/autofill_options.css (right): https://codereview.chromium.org/1583293004/diff/60001/chrome/browser/resource... chrome/browser/resources/options/autofill_options.css:10: height: 192px; /* This is the min-height for lists from WebUI.*/ On 2016/01/28 at 01:35:13, Dan Beam wrote: > i don't understand what this comment adds Just trying not to leave an undocumented Magic Number floating around--it's a seemingly-arbitrary value that's actually dictated by an external constraint. Still, I can remove it if you find it to be more noise than signal. https://codereview.chromium.org/1583293004/diff/60001/chrome/browser/resource... chrome/browser/resources/options/autofill_options.css:60: #autofill-options .extra-margin { On 2016/01/28 at 01:35:13, Dan Beam wrote: > could this just be > > .settings-list + .autofill-section-header { > margin-top: 20px; > } > > and we nuke the extra-margin class? Done.
lgtm
The CQ bit was checked by tmartino@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org Link to the patchset: https://codereview.chromium.org/1583293004/#ps80001 (title: "Removing superfluous extra-margin class")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1583293004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1583293004/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Changes to Autofill settings page to ensure Credit Card list is always at least partially visible on small screens (tested safe to 440px). BUG=575804 ========== to ========== Changes to Autofill settings page to ensure Credit Card list is always at least partially visible on small screens (tested safe to 440px). BUG=575804 Committed: https://crrev.com/4325576e2971f15958ebae063446b72489679dc1 Cr-Commit-Position: refs/heads/master@{#372154} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/4325576e2971f15958ebae063446b72489679dc1 Cr-Commit-Position: refs/heads/master@{#372154} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
