Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2785733003:
MD Settings: Center Pin keyboard in setup pin in settings. (Closed)
|
Created:
3 years, 8 months ago by sammiequon Modified:
3 years, 8 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/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Center Pin keyboard in setup pin in settings.
The pin keyboard and buttons (cancel/continue) are off to the left in settings. It would look better in the middle (and i think it used to). The pin keyboard and buttons were positioned properly in options (and still are after this CL).
BUG=706633
TEST=manual
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2785733003
Cr-Commit-Position: refs/heads/master@{#461146}
Committed: https://chromium.googlesource.com/chromium/src/+/48a2899dec0d18f57fae5b1b6016ad97fe49e1b6
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fixed patch set 1 errors. #
Total comments: 1
Messages
Total messages: 21 (10 generated)
Description was changed from ========== MD Settings: Center Pin keyboard in setup pin in settings. The pin keyboard and buttons (cancel/continue) are off to the left in settings. BUG=706633 TEST=manual ========== to ========== MD Settings: Center Pin keyboard in setup pin in settings. The pin keyboard and buttons (cancel/continue) are off to the left in settings. BUG=706633 TEST=manual CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Center Pin keyboard in setup pin in settings. The pin keyboard and buttons (cancel/continue) are off to the left in settings. BUG=706633 TEST=manual CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Center Pin keyboard in setup pin in settings. The pin keyboard and buttons (cancel/continue) are off to the left in settings. BUG=706633 TEST=manual CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
sammiequon@chromium.org changed reviewers: + jdufault@chromium.org
Description was changed from ========== MD Settings: Center Pin keyboard in setup pin in settings. The pin keyboard and buttons (cancel/continue) are off to the left in settings. BUG=706633 TEST=manual CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Center Pin keyboard in setup pin in settings. The pin keyboard and buttons (cancel/continue) are off to the left in settings. It would look better in the middle (and i think it used to). BUG=706633 TEST=manual CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
jdufault@ - Please take a look. Thanks!
On 2017/03/30 18:15:18, sammiequon wrote: > jdufault@ - Please take a look. Thanks! thanks! lgtm
did you also verify this still works as expected in options?
On 2017/03/30 18:39:05, jdufault wrote: > did you also verify this still works as expected in options? Just tried it (thought options was gone). Looks OK both before and after this fix for some reason.
Description was changed from ========== MD Settings: Center Pin keyboard in setup pin in settings. The pin keyboard and buttons (cancel/continue) are off to the left in settings. It would look better in the middle (and i think it used to). BUG=706633 TEST=manual CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Center Pin keyboard in setup pin in settings. The pin keyboard and buttons (cancel/continue) are off to the left in settings. It would look better in the middle (and i think it used to). The pin keyboard and buttons were positioned properly in options (and still are after this CL). BUG=706633 TEST=manual CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
sammiequon@chromium.org changed reviewers: + stevenjb@chromium.org
stevenjb@ - Please take a look. Thanks!
https://codereview.chromium.org/2785733003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/setup_pin_dialog.html (right): https://codereview.chromium.org/2785733003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/setup_pin_dialog.html:17: } We should really really try to avoid 'float', we should be able to align this correctly. https://codereview.chromium.org/2785733003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/setup_pin_dialog.html:35: .align-center { Making flex part of a class named 'align-center' is confusing. Instead, give the only div that uses this an id and use that here.
https://codereview.chromium.org/2785733003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/setup_pin_dialog.html (right): https://codereview.chromium.org/2785733003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/setup_pin_dialog.html:17: } On 2017/03/30 22:14:17, stevenjb wrote: > We should really really try to avoid 'float', we should be able to align this > correctly. This uses the text-align: end in shared-css now. https://codereview.chromium.org/2785733003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/setup_pin_dialog.html:35: .align-center { On 2017/03/30 22:14:17, stevenjb wrote: > Making flex part of a class named 'align-center' is confusing. Instead, give the > only div that uses this an id and use that here. This is no longer needed after removing </style><style> up there but ill leave it as an id since only one div uses that class.
lgtm https://codereview.chromium.org/2785733003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/setup_pin_dialog.html (left): https://codereview.chromium.org/2785733003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/setup_pin_dialog.html:14: <style> heh, oops!
On 2017/03/31 01:53:34, stevenjb wrote: > lgtm > > https://codereview.chromium.org/2785733003/diff/20001/chrome/browser/resource... > File chrome/browser/resources/settings/people_page/setup_pin_dialog.html (left): > > https://codereview.chromium.org/2785733003/diff/20001/chrome/browser/resource... > chrome/browser/resources/settings/people_page/setup_pin_dialog.html:14: <style> > heh, oops! Thanks!
The CQ bit was checked by sammiequon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jdufault@chromium.org Link to the patchset: https://codereview.chromium.org/2785733003/#ps20001 (title: "Fixed patch set 1 errors.")
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": 20001, "attempt_start_ts": 1490973860998760,
"parent_rev": "965a47b7737c6f15238447d8d8fb7105183acc8a", "commit_rev":
"48a2899dec0d18f57fae5b1b6016ad97fe49e1b6"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Center Pin keyboard in setup pin in settings. The pin keyboard and buttons (cancel/continue) are off to the left in settings. It would look better in the middle (and i think it used to). The pin keyboard and buttons were positioned properly in options (and still are after this CL). BUG=706633 TEST=manual CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Center Pin keyboard in setup pin in settings. The pin keyboard and buttons (cancel/continue) are off to the left in settings. It would look better in the middle (and i think it used to). The pin keyboard and buttons were positioned properly in options (and still are after this CL). BUG=706633 TEST=manual CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2785733003 Cr-Commit-Position: refs/heads/master@{#461146} Committed: https://chromium.googlesource.com/chromium/src/+/48a2899dec0d18f57fae5b1b6016... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/48a2899dec0d18f57fae5b1b6016... |
