Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2848483002:
md settings: Move setup pin error message. (Closed)
|
Created:
3 years, 7 months ago by sammiequon Modified:
3 years, 7 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, alemate+watch_chromium.org, vabr+watchlistpasswordmanager_chromium.org, michaelpg+watch-md-settings_chromium.org, dbeam+watch-closure_chromium.org, michaelpg+watch-md-ui_chromium.org, michaelpg+watch-options_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, vitalyp+closure_chromium.org, dbeam+watch-settings_chromium.org, srahim+watch_chromium.org, stevenjb+watch-md-settings_chromium.org, gcasto+watchlist_chromium.org, jlklein+watch-closure_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionmd settings: Move setup pin error message.
Mocks are here https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZfSHzqDNTRuU/files/MCFvOc4T9u7Awmru3H3e7QDjwMSJbxAVNpw. (page 11/12)
Moves setup pin error/warning message inside pin keyboard element.Have not received positioning/sizing details. But this CL covers the functionality.
Screenshots:
https://screenshot.googleplex.com/qsWXoWteyDr
TEST=browser_tests --gtest_filter="CrSettingsPeoplePageSetupPinDialogTest.*
BUG=703998
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2848483002
Cr-Commit-Position: refs/heads/master@{#472164}
Committed: https://chromium.googlesource.com/chromium/src/+/13ae67b02915295a69a6ceea4ba8c7552b4f29cc
Patch Set 1 #
Total comments: 2
Patch Set 2 : Css ordering. #
Total comments: 2
Patch Set 3 : Fix patch set 2 errors. #
Messages
Total messages: 27 (17 generated)
Description was changed from ========== md settings: Move setup pin error message. BUG= ========== to ========== md settings: Move setup pin error message. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was checked by sammiequon@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Description was changed from ========== md settings: Move setup pin error message. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== md settings: Move setup pin error message. Mocks are here https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZfSHzqDNTRuU/files/MCFvOc.... (page 11/12) Moves setup pin error/warning message inside pin keyboard element.Have not received new icon or positioning/sizing details. But this CL covers the functionality. Screenshots: https://screenshot.googleplex.com/kGeWfciu2gn TEST=browser_tests --gtest_filter="CrSettingsPeoplePageSetupPinDialogTest.* BUG=703998 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:40001) has been deleted
Description was changed from ========== md settings: Move setup pin error message. Mocks are here https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZfSHzqDNTRuU/files/MCFvOc.... (page 11/12) Moves setup pin error/warning message inside pin keyboard element.Have not received new icon or positioning/sizing details. But this CL covers the functionality. Screenshots: https://screenshot.googleplex.com/kGeWfciu2gn TEST=browser_tests --gtest_filter="CrSettingsPeoplePageSetupPinDialogTest.* BUG=703998 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== md settings: Move setup pin error message. Mocks are here https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZfSHzqDNTRuU/files/MCFvOc.... (page 11/12) Moves setup pin error/warning message inside pin keyboard element.Have not received new icon or positioning/sizing details. But this CL covers the functionality. Screenshots: https://screenshot.googleplex.com/qsWXoWteyDr TEST=browser_tests --gtest_filter="CrSettingsPeoplePageSetupPinDialogTest.* BUG=703998 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
sammiequon@chromium.org changed reviewers: + jdufault@chromium.org
Description was changed from ========== md settings: Move setup pin error message. Mocks are here https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZfSHzqDNTRuU/files/MCFvOc.... (page 11/12) Moves setup pin error/warning message inside pin keyboard element.Have not received new icon or positioning/sizing details. But this CL covers the functionality. Screenshots: https://screenshot.googleplex.com/qsWXoWteyDr TEST=browser_tests --gtest_filter="CrSettingsPeoplePageSetupPinDialogTest.* BUG=703998 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== md settings: Move setup pin error message. Mocks are here https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZfSHzqDNTRuU/files/MCFvOc.... (page 11/12) Moves setup pin error/warning message inside pin keyboard element.Have not received positioning/sizing details. But this CL covers the functionality. Screenshots: https://screenshot.googleplex.com/qsWXoWteyDr TEST=browser_tests --gtest_filter="CrSettingsPeoplePageSetupPinDialogTest.* BUG=703998 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
jdufault@ - Please take a look. Thanks!
lgtm https://codereview.chromium.org/2848483002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/setup_pin_dialog.html (right): https://codereview.chromium.org/2848483002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/people_page/setup_pin_dialog.html:31: color: var(--paper-grey-700); Why did you move the warning classes? It'd be nice to keep it next to the error classes.
Thanks! https://codereview.chromium.org/2848483002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/setup_pin_dialog.html (right): https://codereview.chromium.org/2848483002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/people_page/setup_pin_dialog.html:31: color: var(--paper-grey-700); On 2017/05/11 18:55:32, jdufault wrote: > Why did you move the warning classes? It'd be nice to keep it next to the error > classes. Done. I just liked them alphabetized :-P.
sammiequon@chromium.org changed reviewers: + stevenjb@chromium.org
On 2017/05/11 19:03:17, sammiequon wrote: > Thanks! > > https://codereview.chromium.org/2848483002/diff/60001/chrome/browser/resource... > File chrome/browser/resources/settings/people_page/setup_pin_dialog.html > (right): > > https://codereview.chromium.org/2848483002/diff/60001/chrome/browser/resource... > chrome/browser/resources/settings/people_page/setup_pin_dialog.html:31: color: > var(--paper-grey-700); > On 2017/05/11 18:55:32, jdufault wrote: > > Why did you move the warning classes? It'd be nice to keep it next to the > error > > classes. > > Done. I just liked them alphabetized :-P. stevenjb@ - Please take a look. Thanks!
On 2017/05/11 19:04:03, sammiequon wrote: > On 2017/05/11 19:03:17, sammiequon wrote: > > Thanks! > > > > > https://codereview.chromium.org/2848483002/diff/60001/chrome/browser/resource... > > File chrome/browser/resources/settings/people_page/setup_pin_dialog.html > > (right): > > > > > https://codereview.chromium.org/2848483002/diff/60001/chrome/browser/resource... > > chrome/browser/resources/settings/people_page/setup_pin_dialog.html:31: color: > > var(--paper-grey-700); > > On 2017/05/11 18:55:32, jdufault wrote: > > > Why did you move the warning classes? It'd be nice to keep it next to the > > error > > > classes. > > > > Done. I just liked them alphabetized :-P. > > stevenjb@ - Please take a look. Thanks! Friendly ping.
https://codereview.chromium.org/2848483002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/setup_pin_dialog.html (right): https://codereview.chromium.org/2848483002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/setup_pin_dialog.html:49: tests. --> nit: 'Id is needed for tests' is common enough that we generally don't comment it.
lgtm
Thanks! https://codereview.chromium.org/2848483002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/setup_pin_dialog.html (right): https://codereview.chromium.org/2848483002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/setup_pin_dialog.html:49: tests. --> On 2017/05/15 18:51:12, stevenjb wrote: > nit: 'Id is needed for tests' is common enough that we generally don't comment > it. Done.
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, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2848483002/#ps100001 (title: "Fix patch set 2 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": 100001, "attempt_start_ts": 1494952203850310,
"parent_rev": "7527494451ef6967ba4f39c657782b4babccbc7e", "commit_rev":
"13ae67b02915295a69a6ceea4ba8c7552b4f29cc"}
Message was sent while issue was closed.
Description was changed from ========== md settings: Move setup pin error message. Mocks are here https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZfSHzqDNTRuU/files/MCFvOc.... (page 11/12) Moves setup pin error/warning message inside pin keyboard element.Have not received positioning/sizing details. But this CL covers the functionality. Screenshots: https://screenshot.googleplex.com/qsWXoWteyDr TEST=browser_tests --gtest_filter="CrSettingsPeoplePageSetupPinDialogTest.* BUG=703998 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== md settings: Move setup pin error message. Mocks are here https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZfSHzqDNTRuU/files/MCFvOc.... (page 11/12) Moves setup pin error/warning message inside pin keyboard element.Have not received positioning/sizing details. But this CL covers the functionality. Screenshots: https://screenshot.googleplex.com/qsWXoWteyDr TEST=browser_tests --gtest_filter="CrSettingsPeoplePageSetupPinDialogTest.* BUG=703998 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2848483002 Cr-Commit-Position: refs/heads/master@{#472164} Committed: https://chromium.googlesource.com/chromium/src/+/13ae67b02915295a69a6ceea4ba8... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001) as https://chromium.googlesource.com/chromium/src/+/13ae67b02915295a69a6ceea4ba8... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
