Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2338003004:
[MD settings] margin around grid of avatar icons (Closed)
|
Created:
4 years, 3 months ago by dschuyler Modified:
4 years, 3 months ago Reviewers:
tommycli CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, arv+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[MD settings] margin around grid of avatar icons
This CL uses margins rather than max-width to determine when the avatar
icon grid layout should wrap.
BUG=646253
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/b73fa448665be8a0374b14eeb120510f18d97796
Cr-Commit-Position: refs/heads/master@{#419237}
Patch Set 1 #
Total comments: 2
Patch Set 2 : equal margins #
Total comments: 1
Messages
Total messages: 20 (12 generated)
Description was changed from ========== [MD settings] margin around grid of avatar icons This CL uses margins rather than max-width to determine when the avatar icon grid layout should wrap. BUG=646253 ========== to ========== [MD settings] margin around grid of avatar icons This CL uses margins rather than max-width to determine when the avatar icon grid layout should wrap. BUG=646253 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dschuyler@chromium.org changed reviewers: + tommycli@chromium.org
There's a screen shot in the crbug.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
https://codereview.chromium.org/2338003004/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/manage_profile.html (right): https://codereview.chromium.org/2338003004/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/manage_profile.html:13: margin: 16px 24px 16px 20px; does this work in RTL?
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
https://codereview.chromium.org/2338003004/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/manage_profile.html (right): https://codereview.chromium.org/2338003004/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/manage_profile.html:13: margin: 16px 24px 16px 20px; On 2016/09/14 01:13:30, Dan Beam wrote: > does this work in RTL? Good point. It's better to have the margins equal. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
lgtm sans below nit: https://codereview.chromium.org/2338003004/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/manage_profile.html (left): https://codereview.chromium.org/2338003004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/manage_profile.html:15: max-width: 624px; I assume we legitimately don't need max-width anymore?
On 2016/09/16 17:45:46, tommycli wrote: > lgtm sans below nit: > > https://codereview.chromium.org/2338003004/diff/20001/chrome/browser/resource... > File chrome/browser/resources/settings/people_page/manage_profile.html (left): > > https://codereview.chromium.org/2338003004/diff/20001/chrome/browser/resource... > chrome/browser/resources/settings/people_page/manage_profile.html:15: max-width: > 624px; > I assume we legitimately don't need max-width anymore? My thinking is that the new margins, along with a max-width on the section cards will be more maintainable than using a max-width here.
The CQ bit was checked by dschuyler@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [MD settings] margin around grid of avatar icons This CL uses margins rather than max-width to determine when the avatar icon grid layout should wrap. BUG=646253 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] margin around grid of avatar icons This CL uses margins rather than max-width to determine when the avatar icon grid layout should wrap. BUG=646253 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/b73fa448665be8a0374b14eeb120510f18d97796 Cr-Commit-Position: refs/heads/master@{#419237} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/b73fa448665be8a0374b14eeb120510f18d97796 Cr-Commit-Position: refs/heads/master@{#419237} |
