Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2100203003:
Pin keyboard matches red lines (go/cros-quick-unlock-ux). (Closed)
|
Created:
4 years, 5 months ago by sammiequon Modified:
4 years, 5 months ago CC:
chromium-reviews, dzhioev+watch_chromium.org, achuith+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@lkgr Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPin keyboard matches red lines (go/cros-quick-unlock-ux).
BUG=616545
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/9dc6e498d4721b0206777845591a5111e4f86349
Cr-Commit-Position: refs/heads/master@{#403497}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Fixed patch set 2 errors. #
Total comments: 2
Patch Set 3 : Submit color now handled by bind. #
Total comments: 2
Patch Set 4 : Rebased. #Patch Set 5 : Fixed patch set 3 errors. #
Total comments: 1
Messages
Total messages: 21 (8 generated)
Description was changed from ========== Pin keyboard matches red lines (go/cros-quick-unlock-ux). BUG=616545 ========== to ========== Pin keyboard matches red lines (go/cros-quick-unlock-ux). BUG=616545 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Pin keyboard matches red lines (go/cros-quick-unlock-ux). BUG=616545 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Pin keyboard matches red lines (go/cros-quick-unlock-ux). BUG=616545 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
sammiequon@chromium.org changed reviewers: + jdufault@chromium.org
jdufault@ - Please take a look.
https://codereview.chromium.org/2100203003/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2100203003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:30: color: rgb(87, 101, 107); Is there a paper-* color element that this value can be derived from? https://codereview.chromium.org/2100203003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:56: background: rgb(191, 197, 202); Is there a paper-* color element that this value can be derived from? https://codereview.chromium.org/2100203003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:77: color: rgb(191, 197, 202); Is there a paper-* color element that this value can be derived from? https://codereview.chromium.org/2100203003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:129: <paper-button class="digit-button first-row Will this fit on one line? https://codereview.chromium.org/2100203003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:139: <paper-button class="digit-button left-button" on-tap="onNumberTap_" Instead of having two left/right classes, what about applying the margins to the middle button? https://codereview.chromium.org/2100203003/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2100203003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:69: this.changeSubmitColor_(value.length); The color should only be blue if value.length > 0, right? If the user erases the entire PIN it should go back to normal color.
https://codereview.chromium.org/2100203003/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2100203003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:30: color: rgb(87, 101, 107); On 2016/06/27 19:17:52, jdufault wrote: > Is there a paper-* color element that this value can be derived from? Done. https://codereview.chromium.org/2100203003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:56: background: rgb(191, 197, 202); On 2016/06/27 19:17:52, jdufault wrote: > Is there a paper-* color element that this value can be derived from? Done. https://codereview.chromium.org/2100203003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:77: color: rgb(191, 197, 202); On 2016/06/27 19:17:52, jdufault wrote: > Is there a paper-* color element that this value can be derived from? Done. https://codereview.chromium.org/2100203003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:129: <paper-button class="digit-button first-row On 2016/06/27 19:17:52, jdufault wrote: > Will this fit on one line? Done. https://codereview.chromium.org/2100203003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:139: <paper-button class="digit-button left-button" on-tap="onNumberTap_" On 2016/06/27 19:17:52, jdufault wrote: > Instead of having two left/right classes, what about applying the margins to the > middle button? Done.
https://codereview.chromium.org/2100203003/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2100203003/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:55: changeSubmitColor_: function(pinLength) { Stylize the properties by using a computed property with `value` as a parameter. Something like <paper-button class="[[computeSubmitClass_(value)]]"></paper-button>
https://codereview.chromium.org/2100203003/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2100203003/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:55: changeSubmitColor_: function(pinLength) { On 2016/06/29 21:34:15, jdufault wrote: > Stylize the properties by using a computed property with `value` as a parameter. > Something like > > <paper-button class="[[computeSubmitClass_(value)]]"></paper-button> Done.
lgtm after comment is addressed https://codereview.chromium.org/2100203003/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2100203003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:95: return pinLength > 0 ? 'ready-background' : ''; Inline value.length
sammiequon@chromium.org changed reviewers: + xiyuan@chromium.org
xiyuan@ - Please take a look. https://codereview.chromium.org/2100203003/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2100203003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:95: return pinLength > 0 ? 'ready-background' : ''; On 2016/06/30 18:15:48, jdufault wrote: > Inline value.length Done.
https://codereview.chromium.org/2100203003/diff/80001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/2100203003/diff/80001/ui/login/account_picker... ui/login/account_picker/user_pod_row.css:153: left: 20px; Would this work properly in RTL case? e.g. if you are chrome with lang=ar, how would it look like?
On 2016/06/30 22:33:57, xiyuan wrote: > https://codereview.chromium.org/2100203003/diff/80001/ui/login/account_picker... > File ui/login/account_picker/user_pod_row.css (right): > > https://codereview.chromium.org/2100203003/diff/80001/ui/login/account_picker... > ui/login/account_picker/user_pod_row.css:153: left: 20px; > Would this work properly in RTL case? > > e.g. if you are chrome with lang=ar, how would it look like? Yes it works with RTL because the parent is always 40px bigger so right is implicitly 20px as well.
On 2016/07/01 15:16:11, sammiequon wrote: > On 2016/06/30 22:33:57, xiyuan wrote: > > > https://codereview.chromium.org/2100203003/diff/80001/ui/login/account_picker... > > File ui/login/account_picker/user_pod_row.css (right): > > > > > https://codereview.chromium.org/2100203003/diff/80001/ui/login/account_picker... > > ui/login/account_picker/user_pod_row.css:153: left: 20px; > > Would this work properly in RTL case? > > > > e.g. if you are chrome with lang=ar, how would it look like? > > Yes it works with RTL because the parent is always 40px bigger so right is > implicitly 20px as well. Okay. LGTM then.
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/2100203003/#ps80001 (title: "Fixed patch set 3 errors.")
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.
Description was changed from ========== Pin keyboard matches red lines (go/cros-quick-unlock-ux). BUG=616545 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Pin keyboard matches red lines (go/cros-quick-unlock-ux). BUG=616545 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Pin keyboard matches red lines (go/cros-quick-unlock-ux). BUG=616545 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Pin keyboard matches red lines (go/cros-quick-unlock-ux). BUG=616545 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/9dc6e498d4721b0206777845591a5111e4f86349 Cr-Commit-Position: refs/heads/master@{#403497} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/9dc6e498d4721b0206777845591a5111e4f86349 Cr-Commit-Position: refs/heads/master@{#403497} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
