Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2841313002:
md settings: Update lock screen to match new mocks. (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: Update lock screen to match new mocks.
Mocks are here https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZfSHzqDNTRuU/files/MCFvOc4T9u7Awmru3H3e7QDjwMSJbxAVNpw.
This CL :
- Fixes a bug with password prompt paper-input not receiving focus on first open.
- Changes pin_keyboard.html to use paper-input instead of input. (mocks have blue lines and such)
- Rearranged some items on the lock screen page and added some lines.
- Ran cl format --js.
Left the new error message for another CL. (page 11/12 on the mocks)
Screenshots:
https://screenshot.googleplex.com/P4pLToSntY6
https://screenshot.googleplex.com/ooOYgDy71hZ
TEST=browser_tests --gtest_filter="CrSettingsPeoplePageLockScreenTest.*
BUG=703998
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2841313002
Cr-Commit-Position: refs/heads/master@{#470483}
Committed: https://chromium.googlesource.com/chromium/src/+/bafb3e901459a1abc80ef00ec537d6520db03ff6
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fixed patch set 1 errors. #Patch Set 3 : Removed one line. #Patch Set 4 : Rebased. #Patch Set 5 : Slight change. #
Total comments: 5
Patch Set 6 : Fixed patch set 5 errors. #
Total comments: 14
Patch Set 7 : Fixed patch set 6 errors. #
Total comments: 7
Patch Set 8 : Fixed patch set 7 errors. #Patch Set 9 : Rebased. #Messages
Total messages: 55 (30 generated)
Description was changed from ========== md settings: Update lock screen to match new mocks. Mocks are here https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZfSHzqDNTRuU/files/MCFvOc.... This CL : - Fixes a bug with password prompt paper-input not receiving focus on first open. - Changes pin_keyboard.html to use paper-input instead of input. (mocks have blue lines and such) - Rearranged some items on the lock screen page and added some lines. - Ran cl format --js. Left the new error message for another CL. (page 11/12 on the mocks) TEST=browser_tests --gtest_filter="CrSettingsPeoplePageLockScreenTest.* BUG=703998 ========== to ========== md settings: Update lock screen to match new mocks. Mocks are here https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZfSHzqDNTRuU/files/MCFvOc.... This CL : - Fixes a bug with password prompt paper-input not receiving focus on first open. - Changes pin_keyboard.html to use paper-input instead of input. (mocks have blue lines and such) - Rearranged some items on the lock screen page and added some lines. - Ran cl format --js. Left the new error message for another CL. (page 11/12 on the mocks) TEST=browser_tests --gtest_filter="CrSettingsPeoplePageLockScreenTest.* BUG=703998 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
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 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: This issue passed the CQ dry run.
Patchset #1 (id:1) has been deleted
Description was changed from ========== md settings: Update lock screen to match new mocks. Mocks are here https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZfSHzqDNTRuU/files/MCFvOc.... This CL : - Fixes a bug with password prompt paper-input not receiving focus on first open. - Changes pin_keyboard.html to use paper-input instead of input. (mocks have blue lines and such) - Rearranged some items on the lock screen page and added some lines. - Ran cl format --js. Left the new error message for another CL. (page 11/12 on the mocks) TEST=browser_tests --gtest_filter="CrSettingsPeoplePageLockScreenTest.* BUG=703998 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== md settings: Update lock screen to match new mocks. Mocks are here https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZfSHzqDNTRuU/files/MCFvOc.... This CL : - Fixes a bug with password prompt paper-input not receiving focus on first open. - Changes pin_keyboard.html to use paper-input instead of input. (mocks have blue lines and such) - Rearranged some items on the lock screen page and added some lines. - Ran cl format --js. Left the new error message for another CL. (page 11/12 on the mocks) Screenshots: https://screenshot.googleplex.com/P4pLToSntY6 https://screenshot.googleplex.com/SZRoEv5kctD TEST=browser_tests --gtest_filter="CrSettingsPeoplePageLockScreenTest.* BUG=703998 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
sammiequon@chromium.org changed reviewers: + jdufault@chromium.org, stevenjb@chromium.org
Patchset #1 (id:20001) 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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
jdufault@, stevenjb@ - Please take a look. Thanks!
https://codereview.chromium.org/2841313002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2841313002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:247: has-content$="[[hasInput_(value)]]" Are these format changes created from tooling? https://codereview.chromium.org/2841313002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2841313002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:99: value: 0, Are these commas all automatically inserted by tooling? https://codereview.chromium.org/2841313002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:121: this.$$('hr').hidden = true; Provide an id/class?
Based on the screenshot it seems like the lock screen UI has a bunch of extra lines? https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZfSHzqDNTRuU/files/MCFvOc...
On 2017/04/27 18:47:27, jdufault wrote: > Based on the screenshot it seems like the lock screen UI has a bunch of extra > lines? > > https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZfSHzqDNTRuU/files/MCFvOc... Yeah, it does. I feel like fingerprint and smart lock should be their own section thats why i added them.
https://codereview.chromium.org/2841313002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2841313002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:247: has-content$="[[hasInput_(value)]]" On 2017/04/27 18:47:17, jdufault wrote: > Are these format changes created from tooling? The html ones are manual. The style guide says 4 space indent for line wrapping. https://codereview.chromium.org/2841313002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2841313002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:99: value: 0, On 2017/04/27 18:47:17, jdufault wrote: > Are these commas all automatically inserted by tooling? The formatting will squash them on onto one line ie repeatBlah_ : { type: Number, value: 0 }, adding the comma keeps the number of lines the same. https://codereview.chromium.org/2841313002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:121: this.$$('hr').hidden = true; On 2017/04/27 18:47:17, jdufault wrote: > Provide an id/class? Done.
On 2017/04/27 19:17:45, sammiequon wrote: > On 2017/04/27 18:47:27, jdufault wrote: > > Based on the screenshot it seems like the lock screen UI has a bunch of extra > > lines? > > > > > https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZfSHzqDNTRuU/files/MCFvOc... > > Yeah, it does. I feel like fingerprint and smart lock should be their own > section thats why i added them. I'd recommend discussing your concerns with the visual designer; we should implement to spec (and change the spec if needed).
On 2017/04/27 19:53:36, jdufault wrote: > On 2017/04/27 19:17:45, sammiequon wrote: > > On 2017/04/27 18:47:27, jdufault wrote: > > > Based on the screenshot it seems like the lock screen UI has a bunch of > extra > > > lines? > > > > > > > > > https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZfSHzqDNTRuU/files/MCFvOc... > > > > Yeah, it does. I feel like fingerprint and smart lock should be their own > > section thats why i added them. > > I'd recommend discussing your concerns with the visual designer; we should > implement to spec (and change the spec if needed). I discussed with elizabethchiu@ and she told me to remove the line between the screen lock options & fingerprint, which I did.
On 2017/04/27 22:40:21, sammiequon wrote: > On 2017/04/27 19:53:36, jdufault wrote: > > On 2017/04/27 19:17:45, sammiequon wrote: > > > On 2017/04/27 18:47:27, jdufault wrote: > > > > Based on the screenshot it seems like the lock screen UI has a bunch of > > extra > > > > lines? > > > > > > > > > > > > > > https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZfSHzqDNTRuU/files/MCFvOc... > > > > > > Yeah, it does. I feel like fingerprint and smart lock should be their own > > > section thats why i added them. > > > > I'd recommend discussing your concerns with the visual designer; we should > > implement to spec (and change the spec if needed). > > I discussed with elizabethchiu@ and she told me to remove the line between the > screen lock options & fingerprint, which I did. Can you update the screenshots in the description?
Description was changed from ========== md settings: Update lock screen to match new mocks. Mocks are here https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZfSHzqDNTRuU/files/MCFvOc.... This CL : - Fixes a bug with password prompt paper-input not receiving focus on first open. - Changes pin_keyboard.html to use paper-input instead of input. (mocks have blue lines and such) - Rearranged some items on the lock screen page and added some lines. - Ran cl format --js. Left the new error message for another CL. (page 11/12 on the mocks) Screenshots: https://screenshot.googleplex.com/P4pLToSntY6 https://screenshot.googleplex.com/SZRoEv5kctD TEST=browser_tests --gtest_filter="CrSettingsPeoplePageLockScreenTest.* BUG=703998 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== md settings: Update lock screen to match new mocks. Mocks are here https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZfSHzqDNTRuU/files/MCFvOc.... This CL : - Fixes a bug with password prompt paper-input not receiving focus on first open. - Changes pin_keyboard.html to use paper-input instead of input. (mocks have blue lines and such) - Rearranged some items on the lock screen page and added some lines. - Ran cl format --js. Left the new error message for another CL. (page 11/12 on the mocks) Screenshots: https://screenshot.googleplex.com/P4pLToSntY6 https://screenshot.googleplex.com/a5jrHTDaZRN TEST=browser_tests --gtest_filter="CrSettingsPeoplePageLockScreenTest.* BUG=703998 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
On 2017/05/01 21:15:21, jdufault wrote: > On 2017/04/27 22:40:21, sammiequon wrote: > > On 2017/04/27 19:53:36, jdufault wrote: > > > On 2017/04/27 19:17:45, sammiequon wrote: > > > > On 2017/04/27 18:47:27, jdufault wrote: > > > > > Based on the screenshot it seems like the lock screen UI has a bunch of > > > extra > > > > > lines? > > > > > > > > > > > > > > > > > > > > https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZfSHzqDNTRuU/files/MCFvOc... > > > > > > > > Yeah, it does. I feel like fingerprint and smart lock should be their own > > > > section thats why i added them. > > > > > > I'd recommend discussing your concerns with the visual designer; we should > > > implement to spec (and change the spec if needed). > > > > I discussed with elizabethchiu@ and she told me to remove the line between the > > screen lock options & fingerprint, which I did. > > Can you update the screenshots in the description? Done.
Description was changed from ========== md settings: Update lock screen to match new mocks. Mocks are here https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZfSHzqDNTRuU/files/MCFvOc.... This CL : - Fixes a bug with password prompt paper-input not receiving focus on first open. - Changes pin_keyboard.html to use paper-input instead of input. (mocks have blue lines and such) - Rearranged some items on the lock screen page and added some lines. - Ran cl format --js. Left the new error message for another CL. (page 11/12 on the mocks) Screenshots: https://screenshot.googleplex.com/P4pLToSntY6 https://screenshot.googleplex.com/a5jrHTDaZRN TEST=browser_tests --gtest_filter="CrSettingsPeoplePageLockScreenTest.* BUG=703998 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== md settings: Update lock screen to match new mocks. Mocks are here https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZfSHzqDNTRuU/files/MCFvOc.... This CL : - Fixes a bug with password prompt paper-input not receiving focus on first open. - Changes pin_keyboard.html to use paper-input instead of input. (mocks have blue lines and such) - Rearranged some items on the lock screen page and added some lines. - Ran cl format --js. Left the new error message for another CL. (page 11/12 on the mocks) Screenshots: https://screenshot.googleplex.com/P4pLToSntY6 https://screenshot.googleplex.com/ooOYgDy71hZ TEST=browser_tests --gtest_filter="CrSettingsPeoplePageLockScreenTest.* BUG=703998 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #5 (id:120001) has been deleted
Jacob, please take another look. The latest screen shot has been approved by tbuckley@ :-P.
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: This issue passed the CQ dry run.
We want to use 'SET UP PIN' instead of just 'SET UP'?
https://codereview.chromium.org/2841313002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/lock_screen.html (right): https://codereview.chromium.org/2841313002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/lock_screen.html:38: line-height: 154%; It may be more clear to specify this in pixels? https://codereview.chromium.org/2841313002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/lock_screen.html:59: <h2> $i18n{lockScreenOptions} </h2> spacing after start tag and before end tag (<h2>foo</h2>)
lgtm after comments
https://codereview.chromium.org/2841313002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/lock_screen.html (right): https://codereview.chromium.org/2841313002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/lock_screen.html:38: line-height: 154%; On 2017/05/04 21:11:58, jdufault wrote: > It may be more clear to specify this in pixels? I took this value from settings_var_css. Its used in the other polymer mixins. https://codereview.chromium.org/2841313002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/lock_screen.html:59: <h2> $i18n{lockScreenOptions} </h2> On 2017/05/04 21:11:58, jdufault wrote: > spacing after start tag and before end tag (<h2>foo</h2>) Done.
https://codereview.chromium.org/2841313002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/lock_screen.html (right): https://codereview.chromium.org/2841313002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/lock_screen.html:38: line-height: 154%; On 2017/05/04 23:55:37, sammiequon wrote: > On 2017/05/04 21:11:58, jdufault wrote: > > It may be more clear to specify this in pixels? > > I took this value from settings_var_css. Its used in the other polymer mixins. Acknowledged.
stevenjb@ - Please take a look. Thanks!
https://codereview.chromium.org/2841313002/diff/160001/chrome/app/settings_st... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2841313002/diff/160001/chrome/app/settings_st... chrome/app/settings_strings.grdp:2386: Set Up Elsewhere (e.g. EASY_UNLOCK_SETUP) we use "Set up" which I think is more correct. We should be consistent regardless. https://codereview.chromium.org/2841313002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2841313002/diff/160001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:183: on-keydown="onInputKeyDown_"> hidden="[[!showPinInput_]]" (see comment in JS) https://codereview.chromium.org/2841313002/diff/160001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:186: <hr id="divider"> We only use <hr> one other place. Can we get the same effect with a border on the div below, or <div class="separator">? If UX has confirmed this, that's fine, but we should only use this if it is exactly what UX wants and a border won't give the same effect. Also, instead of using an id, use hidden="[[showPinInput_]]" (see comment in JS) https://codereview.chromium.org/2841313002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2841313002/diff/160001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:121: this.$.divider.hidden = true; Instead use a property: this.showPinInput_ = inputElement == this.$.pinInput.inputElement; https://codereview.chromium.org/2841313002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/lock_screen.html (right): https://codereview.chromium.org/2841313002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/lock_screen.html:40: line-height: 154%; Add comment on where 154% is coming from. https://codereview.chromium.org/2841313002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/lock_screen.html:66: class="settings-box first two-line"> This is weird. We use settings-box for divs, and not generally inside a list-frame. We should be able to get the desired effect using shared vars instead, and then we can use it for all paper-radio-button (or all #unloockType > paper-radio-button). https://codereview.chromium.org/2841313002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/lock_screen.html:119: on-tap="onEditFingerprints_" Usually we want the entire div to handle the on-tap, was this moved intentionally?
https://codereview.chromium.org/2841313002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/lock_screen.html (right): https://codereview.chromium.org/2841313002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/lock_screen.html:119: on-tap="onEditFingerprints_" On 2017/05/08 18:12:22, stevenjb wrote: > Usually we want the entire div to handle the on-tap, was this moved > intentionally? Please ignore. Looking at the mocks (and more closely at the html) I see that this is now a secondary button so we don't want the the entire line to handle the tap.
https://codereview.chromium.org/2841313002/diff/160001/chrome/app/settings_st... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2841313002/diff/160001/chrome/app/settings_st... chrome/app/settings_strings.grdp:2386: Set Up On 2017/05/08 18:12:21, stevenjb wrote: > Elsewhere (e.g. EASY_UNLOCK_SETUP) we use "Set up" which I think is more > correct. We should be consistent regardless. Done. https://codereview.chromium.org/2841313002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2841313002/diff/160001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:183: on-keydown="onInputKeyDown_"> On 2017/05/08 18:12:21, stevenjb wrote: > hidden="[[!showPinInput_]]" (see comment in JS) Done. https://codereview.chromium.org/2841313002/diff/160001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:186: <hr id="divider"> On 2017/05/08 18:12:21, stevenjb wrote: > We only use <hr> one other place. Can we get the same effect with a border on > the div below, or <div class="separator">? If UX has confirmed this, that's > fine, but we should only use this if it is exactly what UX wants and a border > won't give the same effect. > > Also, instead of using an id, use hidden="[[showPinInput_]]" (see comment in JS) Done. https://codereview.chromium.org/2841313002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2841313002/diff/160001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:121: this.$.divider.hidden = true; On 2017/05/08 18:12:21, stevenjb wrote: > Instead use a property: > > this.showPinInput_ = inputElement == this.$.pinInput.inputElement; Done. https://codereview.chromium.org/2841313002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/lock_screen.html (right): https://codereview.chromium.org/2841313002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/lock_screen.html:40: line-height: 154%; On 2017/05/08 18:12:22, stevenjb wrote: > Add comment on where 154% is coming from. Done. https://codereview.chromium.org/2841313002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/lock_screen.html:66: class="settings-box first two-line"> On 2017/05/08 18:12:22, stevenjb wrote: > This is weird. We use settings-box for divs, and not generally inside a > list-frame. We should be able to get the desired effect using shared vars > instead, and then we can use it for all paper-radio-button (or all #unloockType > > paper-radio-button). > how about list-item underbar? https://codereview.chromium.org/2841313002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2841313002/diff/180001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:178: value="{{value}}" Added this because previously, using the clear button/digit buttons would not remove/show the placeholder.
lgtm w/ nits https://codereview.chromium.org/2841313002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2841313002/diff/180001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:160: border-bottom: 1px solid #000; @apply(--cr-separator-line) ? Opacity is .06 instead of .14 but it seems like we should be consistent? (If not, maybe just override opacity). https://codereview.chromium.org/2841313002/diff/180001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:164: width: 160px; Using a fixed width is less than ideal, but is maybe OK in a dialog? Make sure this works regardless of screen width. https://codereview.chromium.org/2841313002/diff/180001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:178: value="{{value}}" On 2017/05/08 19:16:49, sammiequon wrote: > Added this because previously, using the clear button/digit buttons would not > remove/show the placeholder. Does [[value]] work? I'd be worried that the two-way binding would race with handleInputChanged_?
Thanks! https://codereview.chromium.org/2841313002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2841313002/diff/180001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:160: border-bottom: 1px solid #000; On 2017/05/08 19:30:30, stevenjb wrote: > @apply(--cr-separator-line) ? Opacity is .06 instead of .14 but it seems like we > should be consistent? (If not, maybe just override opacity). > Done. https://codereview.chromium.org/2841313002/diff/180001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:164: width: 160px; On 2017/05/08 19:30:30, stevenjb wrote: > Using a fixed width is less than ideal, but is maybe OK in a dialog? Make sure > this works regardless of screen width. I think the dialog is fixed at 180px so this works but margin: 0 10px would probably be more resistant to change. Done. https://codereview.chromium.org/2841313002/diff/180001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:178: value="{{value}}" On 2017/05/08 19:30:30, stevenjb wrote: > On 2017/05/08 19:16:49, sammiequon wrote: > > Added this because previously, using the clear button/digit buttons would not > > remove/show the placeholder. > > Does [[value]] work? I'd be worried that the two-way binding would race with > handleInputChanged_? Works. Done.
Patchset #9 (id:220001) has been deleted
Patchset #9 (id:240001) has been deleted
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/2841313002/#ps260001 (title: "Rebased.")
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
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by sammiequon@chromium.org
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": 260001, "attempt_start_ts": 1494390602454770,
"parent_rev": "48358e269fbafbae5df07b4a82f0a7762f1ce19b", "commit_rev":
"bafb3e901459a1abc80ef00ec537d6520db03ff6"}
Message was sent while issue was closed.
Description was changed from ========== md settings: Update lock screen to match new mocks. Mocks are here https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZfSHzqDNTRuU/files/MCFvOc.... This CL : - Fixes a bug with password prompt paper-input not receiving focus on first open. - Changes pin_keyboard.html to use paper-input instead of input. (mocks have blue lines and such) - Rearranged some items on the lock screen page and added some lines. - Ran cl format --js. Left the new error message for another CL. (page 11/12 on the mocks) Screenshots: https://screenshot.googleplex.com/P4pLToSntY6 https://screenshot.googleplex.com/ooOYgDy71hZ TEST=browser_tests --gtest_filter="CrSettingsPeoplePageLockScreenTest.* BUG=703998 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== md settings: Update lock screen to match new mocks. Mocks are here https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZfSHzqDNTRuU/files/MCFvOc.... This CL : - Fixes a bug with password prompt paper-input not receiving focus on first open. - Changes pin_keyboard.html to use paper-input instead of input. (mocks have blue lines and such) - Rearranged some items on the lock screen page and added some lines. - Ran cl format --js. Left the new error message for another CL. (page 11/12 on the mocks) Screenshots: https://screenshot.googleplex.com/P4pLToSntY6 https://screenshot.googleplex.com/ooOYgDy71hZ TEST=browser_tests --gtest_filter="CrSettingsPeoplePageLockScreenTest.* BUG=703998 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2841313002 Cr-Commit-Position: refs/heads/master@{#470483} Committed: https://chromium.googlesource.com/chromium/src/+/bafb3e901459a1abc80ef00ec537... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:260001) as https://chromium.googlesource.com/chromium/src/+/bafb3e901459a1abc80ef00ec537... |
