Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2675493002:
MD Settings: Touch up cr-dialog footers (Closed)
|
Created:
3 years, 10 months ago by tommycli Modified:
3 years, 10 months ago 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. |
DescriptionMD Settings: Touch up cr-dialog footers
Makes the footer white background color with an optional gray top border.
BUG=684152
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2675493002
Cr-Commit-Position: refs/heads/master@{#447800}
Committed: https://chromium.googlesource.com/chromium/src/+/2df23785add5036585c322687025c83e03cb5764
Patch Set 1 #
Total comments: 2
Patch Set 2 : fix dialogs #
Total comments: 4
Patch Set 3 : fix shit up #
Messages
Total messages: 26 (14 generated)
Description was changed from ========== MD Settings: Touch up Sign Out dialog Makes the footer white background color with a gray top border. BUG=684152 ========== to ========== MD Settings: Touch up Sign Out dialog Makes the footer white background color with a gray top border. BUG=684152 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
tommycli@chromium.org changed reviewers: + scottchen@chromium.org
scottchen: PTAL, thanks! https://codereview.chromium.org/2675493002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/people_page.html (right): https://codereview.chromium.org/2675493002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/people_page.html:100: border-top: 1px solid rgba(0, 0, 0, 0.14); Spreadsheet calls for 0.12 alpha, but this seems like it should be exactly the same as top separator line defined here: https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_dialog... Has same TODO question as top separator line, but i think we can address those TODOs later together.
On 2017/02/01 22:46:38, tommycli wrote: > scottchen: PTAL, thanks! > > https://codereview.chromium.org/2675493002/diff/1/chrome/browser/resources/se... > File chrome/browser/resources/settings/people_page/people_page.html (right): > > https://codereview.chromium.org/2675493002/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/people_page/people_page.html:100: border-top: > 1px solid rgba(0, 0, 0, 0.14); > Spreadsheet calls for 0.12 alpha, but this seems like it should be exactly the > same as top separator line defined here: > > https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_dialog... > > Has same TODO question as top separator line, but i think we can address those > TODOs later together. Before and after pics: http://imgur.com/a/iARuW
On 2017/02/01 22:47:53, tommycli wrote: > On 2017/02/01 22:46:38, tommycli wrote: > > scottchen: PTAL, thanks! > > > > > https://codereview.chromium.org/2675493002/diff/1/chrome/browser/resources/se... > > File chrome/browser/resources/settings/people_page/people_page.html (right): > > > > > https://codereview.chromium.org/2675493002/diff/1/chrome/browser/resources/se... > > chrome/browser/resources/settings/people_page/people_page.html:100: > border-top: > > 1px solid rgba(0, 0, 0, 0.14); > > Spreadsheet calls for 0.12 alpha, but this seems like it should be exactly the > > same as top separator line defined here: > > > > > https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_dialog... > > > > Has same TODO question as top separator line, but i think we can address those > > TODOs later together. > > Before and after pics: http://imgur.com/a/iARuW lgtm
The CQ bit was checked by tommycli@chromium.org
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
tommycli@chromium.org changed reviewers: + dbeam@chromium.org
dbeam: PTAL thanks!
lgtm but can you change .footer in cr-dialog itself if possible? (to not have a gray background) https://codereview.chromium.org/2675493002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/people_page.html (right): https://codereview.chromium.org/2675493002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/people_page.html:100: border-top: 1px solid rgba(0, 0, 0, 0.14); On 2017/02/01 22:46:38, tommycli wrote: > Spreadsheet calls for 0.12 alpha, but this seems like it should be exactly the > same as top separator line defined here: > > https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_dialog... > > Has same TODO question as top separator line, but i think we can address those > TODOs later together. aight, maybe --divider-color too
Description was changed from ========== MD Settings: Touch up Sign Out dialog Makes the footer white background color with a gray top border. BUG=684152 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Touch up cr-dialog footers Makes the footer white background color with an optional gray top border. BUG=684152 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dbeam: PTAL again. I made the change global to cr-dialog. I confirmed that only 3 dialogs (all in settings) made use of the dialog footer. The History and Downloads cr-dialog should be unaffected. Both Reset and Sign Out dialogs (but not CBD) have the gray border on the bottom so I made that optional. I'm pretty sure CBD intentionally does not have that footer border because it looks awful with it.
The CQ bit was checked by tommycli@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...
lgtm https://codereview.chromium.org/2675493002/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right): https://codereview.chromium.org/2675493002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:34: border-bottom-color: var(--divider-color); border-bottom: 1px solid var(--divider-color); https://codereview.chromium.org/2675493002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:96: border-top-color: var(--divider-color); border-top: 1px solid var(--divider-color);
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
thanks! https://codereview.chromium.org/2675493002/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right): https://codereview.chromium.org/2675493002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:34: border-bottom-color: var(--divider-color); On 2017/02/02 03:59:29, Dan Beam (slow) wrote: > border-bottom: 1px solid var(--divider-color); Done. Okay awesome I did not think that would work, but I have confirmed it does. https://codereview.chromium.org/2675493002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:96: border-top-color: var(--divider-color); On 2017/02/02 03:59:29, Dan Beam (slow) wrote: > border-top: 1px solid var(--divider-color); Done.
The CQ bit was checked by tommycli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scottchen@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2675493002/#ps40001 (title: "fix shit up")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486055433738060,
"parent_rev": "5df2995602b5410c8b1143eac641b05ff1874147", "commit_rev":
"2df23785add5036585c322687025c83e03cb5764"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Touch up cr-dialog footers Makes the footer white background color with an optional gray top border. BUG=684152 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Touch up cr-dialog footers Makes the footer white background color with an optional gray top border. BUG=684152 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2675493002 Cr-Commit-Position: refs/heads/master@{#447800} Committed: https://chromium.googlesource.com/chromium/src/+/2df23785add5036585c322687025... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/2df23785add5036585c322687025... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
