Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2222583002:
Lock screen pin keyboard ui upgrades. (Closed)
|
Created:
4 years, 4 months ago by sammiequon Modified:
4 years, 3 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. |
DescriptionLock screen pin keyboard ui upgrades.
Changed the pin keyboard to match the specs at https://folio.googleplex.com/cros-core-ui/spec#%2F5.1%20-%20Start%20screen%20Pin%20lock%20pods.png%3Fz=width, minus the submit button on the normal pod and the errors on the pin keyboard.
BUG=627222
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/12bda3871f8b16b531430d0a7db60ba4468d6c11
Cr-Commit-Position: refs/heads/master@{#414858}
Patch Set 1 #
Total comments: 22
Patch Set 2 : Fixed patch set 1 errors. #
Total comments: 10
Patch Set 3 : Fixed patch set 2 errors. #
Total comments: 8
Patch Set 4 : Rebased. #Patch Set 5 : Fixed patch set 3 errors. #
Total comments: 6
Patch Set 6 : Pin warnings moved inside pin keyboard. #
Total comments: 12
Patch Set 7 : Fixed patch set 6 errors. #
Total comments: 14
Patch Set 8 : Rebased. #Patch Set 9 : Removed problem message in pin keyboard. #
Total comments: 4
Patch Set 10 : Fixed patch set 9 errors. #
Total comments: 2
Patch Set 11 : Fixed patch set 10 errors. #
Total comments: 6
Patch Set 12 : Nits. #Patch Set 13 : Fix for closure. #
Messages
Total messages: 50 (20 generated)
Description was changed from ========== Lock screen pin keyboard ui upgrades. BUG=627222 ========== to ========== Lock screen pin keyboard ui upgrades. BUG=627222 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Lock screen pin keyboard ui upgrades. BUG=627222 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Lock screen pin keyboard ui upgrades. BUG=627222 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
sammiequon@chromium.org changed reviewers: + jdufault@chromium.org
jdufault@ - Please take a look.
Did you also test this on chrome://md-settings? https://codereview.chromium.org/2222583002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2222583002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:119: #pin-seperator { nit: sp separator https://codereview.chromium.org/2222583002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:159: <hr id="pin-seperator"> Apply the css to all <hr> elements instead of making an id just for this element. https://codereview.chromium.org/2222583002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:160: <div class="row keyboard"> Can the top-row-button css class be applied to this div instead (and repeat for bottom-row-button) https://codereview.chromium.org/2222583002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:205: on-tap="onPinClear_" icon="icons:backspace"> Align with the line above https://codereview.chromium.org/2222583002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2222583002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:130: * Changes the color of input text if a something is entered. What about: Changes the color of the input text if it is not empty. https://codereview.chromium.org/2222583002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:143: getButtonsDisabled_: function(value) { Rename to hasInput_ https://codereview.chromium.org/2222583002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:144: return value.length > 0 ? false : true; return value.length == 0; https://codereview.chromium.org/2222583002/diff/40001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/2222583002/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.css:63: width: 180px; Does width need to be specified? https://codereview.chromium.org/2222583002/diff/40001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2222583002/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:62: var CROS_PIN_POD_WIDTH = 180; Can we eliminate this variable entirely, or set it to another constant value? https://codereview.chromium.org/2222583002/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:2555: this.pinKeyboardLoaded_ = true; Can we just query the DOM and see if there is content inside of the PIN keyboard? Why did this break, since it was working w/o this before? https://codereview.chromium.org/2222583002/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:3129: }, Why the removal?
Patchset #2 (id:60001) has been deleted
https://codereview.chromium.org/2222583002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2222583002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:119: #pin-seperator { On 2016/08/05 21:58:51, jdufault wrote: > nit: sp separator Done. https://codereview.chromium.org/2222583002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:159: <hr id="pin-seperator"> On 2016/08/05 21:58:51, jdufault wrote: > Apply the css to all <hr> elements instead of making an id just for this > element. Done. https://codereview.chromium.org/2222583002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:160: <div class="row keyboard"> On 2016/08/05 21:58:51, jdufault wrote: > Can the top-row-button css class be applied to this div instead (and repeat for > bottom-row-button) Done. https://codereview.chromium.org/2222583002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:205: on-tap="onPinClear_" icon="icons:backspace"> On 2016/08/05 21:58:51, jdufault wrote: > Align with the line above Done. https://codereview.chromium.org/2222583002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2222583002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:130: * Changes the color of input text if a something is entered. On 2016/08/05 21:58:52, jdufault wrote: > What about: > > Changes the color of the input text if it is not empty. Done. https://codereview.chromium.org/2222583002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:143: getButtonsDisabled_: function(value) { On 2016/08/05 21:58:52, jdufault wrote: > Rename to hasInput_ Done. https://codereview.chromium.org/2222583002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:144: return value.length > 0 ? false : true; On 2016/08/05 21:58:51, jdufault wrote: > return value.length == 0; Done. https://codereview.chromium.org/2222583002/diff/40001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/2222583002/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.css:63: width: 180px; On 2016/08/05 21:58:52, jdufault wrote: > Does width need to be specified? Done. https://codereview.chromium.org/2222583002/diff/40001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2222583002/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:62: var CROS_PIN_POD_WIDTH = 180; On 2016/08/05 21:58:52, jdufault wrote: > Can we eliminate this variable entirely, or set it to another constant value? Done. https://codereview.chromium.org/2222583002/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:2555: this.pinKeyboardLoaded_ = true; On 2016/08/05 21:58:52, jdufault wrote: > Can we just query the DOM and see if there is content inside of the PIN > keyboard? > > Why did this break, since it was working w/o this before? Done. For multiple pods we want the keyboard to expand on focus, so it was expanding right away before loading the keyboard. https://codereview.chromium.org/2222583002/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:3129: }, On 2016/08/05 21:58:52, jdufault wrote: > Why the removal? This was added to support multiple pods and to re position their lefts and widths after a pin keyboard is shown/hidden. This is no longer needed because the widths are constant.
https://codereview.chromium.org/2222583002/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2222583002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:109: #pin-input.ready-background { What about 'has-content' instead of 'ready-background'? Then rename getPasswordReadyClass_ to getContentClass_. https://codereview.chromium.org/2222583002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:159: <hr> <hr /> https://codereview.chromium.org/2222583002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:199: disabled="[[hasNoInput_(value)]]" !hasInput_(value) Generally prefer not having the negative inside of the function name so that we can reduce the number of double-negatives (!hasNoInput_) https://codereview.chromium.org/2222583002/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2222583002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:130: * Changes the color of input text if it is not empty. of the* input https://codereview.chromium.org/2222583002/diff/80001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2222583002/diff/80001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1095: if (this.pinKeyboard) return this.pinKeyboard && this.pinKeyboard.offsetHeight > 0;
https://codereview.chromium.org/2222583002/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2222583002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:109: #pin-input.ready-background { On 2016/08/08 21:12:42, jdufault wrote: > What about 'has-content' instead of 'ready-background'? Then rename > getPasswordReadyClass_ to getContentClass_. Done. https://codereview.chromium.org/2222583002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:159: <hr> On 2016/08/08 21:12:42, jdufault wrote: > <hr /> Presumbit says do not close single tags. https://codereview.chromium.org/2222583002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:199: disabled="[[hasNoInput_(value)]]" On 2016/08/08 21:12:42, jdufault wrote: > !hasInput_(value) > > Generally prefer not having the negative inside of the function name so that we > can reduce the number of double-negatives (!hasNoInput_) Done. https://codereview.chromium.org/2222583002/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2222583002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:130: * Changes the color of input text if it is not empty. On 2016/08/08 21:12:42, jdufault wrote: > of the* input Done. https://codereview.chromium.org/2222583002/diff/80001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2222583002/diff/80001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1095: if (this.pinKeyboard) On 2016/08/08 21:12:42, jdufault wrote: > return this.pinKeyboard && this.pinKeyboard.offsetHeight > 0; Done.
https://codereview.chromium.org/2222583002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2222583002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:182: <div class$="error-message [[getErrorClass_(enableError)]]"> Use a <dom-if> instead of the error class. <template is="dom-if" if="[[enableError]]"> <!-- ... --> </template> https://codereview.chromium.org/2222583002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:182: <div class$="error-message [[getErrorClass_(enableError)]]"> The error message also needs to be customizable. Instead of enableError, just have an errorMessage property. https://codereview.chromium.org/2222583002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:183: Pin or password incorrect. Make sure to localize any strings you include. https://codereview.chromium.org/2222583002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2222583002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:51: * Whether or not to show an erro. nit: error
Patchset #5 (id:140001) has been deleted
https://codereview.chromium.org/2222583002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2222583002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:182: <div class$="error-message [[getErrorClass_(enableError)]]"> On 2016/08/08 23:47:25, jdufault wrote: > Use a <dom-if> instead of the error class. > > <template is="dom-if" if="[[enableError]]"> > <!-- ... --> > </template> Done. https://codereview.chromium.org/2222583002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:182: <div class$="error-message [[getErrorClass_(enableError)]]"> On 2016/08/08 23:47:25, jdufault wrote: > The error message also needs to be customizable. Instead of enableError, just > have an errorMessage property. Added a public function in pin_keyboard.js which can change this. https://codereview.chromium.org/2222583002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:183: Pin or password incorrect. On 2016/08/08 23:47:25, jdufault wrote: > Make sure to localize any strings you include. Done. https://codereview.chromium.org/2222583002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2222583002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:51: * Whether or not to show an erro. On 2016/08/08 23:47:25, jdufault wrote: > nit: error Done.
Description was changed from ========== Lock screen pin keyboard ui upgrades. BUG=627222 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Lock screen pin keyboard ui upgrades. Changed the pin keyboard to match the specs at https://folio.googleplex.com/cros-core-ui/spec#%2F5.1%20-%20Start%20screen%20..., minus the submit button on the normal pod and the errors on the pin keyboard. BUG=627222 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
sammiequon@chromium.org changed reviewers: + xiyuan@chromium.org
On 2016/08/11 01:03:07, sammiequon wrote: > https://codereview.chromium.org/2222583002/diff/100001/chrome/browser/resourc... > File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): > > https://codereview.chromium.org/2222583002/diff/100001/chrome/browser/resourc... > chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:182: <div > class$="error-message [[getErrorClass_(enableError)]]"> > On 2016/08/08 23:47:25, jdufault wrote: > > Use a <dom-if> instead of the error class. > > > > <template is="dom-if" if="[[enableError]]"> > > <!-- ... --> > > </template> > > Done. > > https://codereview.chromium.org/2222583002/diff/100001/chrome/browser/resourc... > chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:182: <div > class$="error-message [[getErrorClass_(enableError)]]"> > On 2016/08/08 23:47:25, jdufault wrote: > > The error message also needs to be customizable. Instead of enableError, just > > have an errorMessage property. > > Added a public function in pin_keyboard.js which can change this. > > https://codereview.chromium.org/2222583002/diff/100001/chrome/browser/resourc... > chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:183: Pin or > password incorrect. > On 2016/08/08 23:47:25, jdufault wrote: > > Make sure to localize any strings you include. > > Done. > > https://codereview.chromium.org/2222583002/diff/100001/chrome/browser/resourc... > File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): > > https://codereview.chromium.org/2222583002/diff/100001/chrome/browser/resourc... > chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:51: * Whether or > not to show an erro. > On 2016/08/08 23:47:25, jdufault wrote: > > nit: error > > Done. xiyuan@ - Please take a look. Thanks!
https://codereview.chromium.org/2222583002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2222583002/diff/160001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:94: position: absolute; Can you use flexbox for this?
https://codereview.chromium.org/2222583002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2222583002/diff/160001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:94: position: absolute; On 2016/08/18 17:44:55, jdufault wrote: > Can you use flexbox for this? This will get removed in https://codereview.chromium.org/2254623003/ and its replacement will use flexbox if possible.
https://codereview.chromium.org/2222583002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2222583002/diff/160001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:200: $i18n{pinKeyboardErrorMessage} Instead of setMessage just bind this value using a property, ie, [[errorMessage]]. enableError can then be computed by checking !!errorMessage https://codereview.chromium.org/2222583002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2222583002/diff/160001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:84: setMessage: function(newMessage) { Remove this, use a property instead.
Patchset #6 (id:180001) has been deleted
https://codereview.chromium.org/2222583002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2222583002/diff/160001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:200: $i18n{pinKeyboardErrorMessage} On 2016/08/19 17:47:57, jdufault wrote: > Instead of setMessage just bind this value using a property, ie, > [[errorMessage]]. > > enableError can then be computed by checking !!errorMessage Binded, but !! does not seem to work. https://codereview.chromium.org/2222583002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2222583002/diff/160001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:84: setMessage: function(newMessage) { On 2016/08/19 17:47:57, jdufault wrote: > Remove this, use a property instead. Done.
https://codereview.chromium.org/2222583002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2222583002/diff/200001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:204: <div id="error-message" class$="[getErrorDirection_]"> [[getErrorDirection_()]] https://codereview.chromium.org/2222583002/diff/200001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:205: [[errorMessage]] Please also add support for warnings. I'd rename errorMessage to problemMessage_ and have a problemClass_='warning'/'error' https://codereview.chromium.org/2222583002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2222583002/diff/200001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:194: * Computes the direction of the errorMessage. nit: error message https://codereview.chromium.org/2222583002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/setup_pin_dialog.js (right): https://codereview.chromium.org/2222583002/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/setup_pin_dialog.js:157: this.pinKeyboard.errorMessage = this.i18n(messageId); what about this for a public API? this.pinKeyboard.setProblem(this.i18n(messageId), true /*isError*/); https://codereview.chromium.org/2222583002/diff/200001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/2222583002/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:381: builder->Add("pinKeyboardErrorMessage", IDS_PIN_KEYBOARD_ERROR_MESSAGE); Move string to oobe strings, rename it so it is specific to a failed login. https://codereview.chromium.org/2222583002/diff/200001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc (right): https://codereview.chromium.org/2222583002/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:876: {"pinKeyboardErrorMessage", IDS_PIN_KEYBOARD_ERROR_MESSAGE}, Remove this after the change above.
Patchset #7 (id:220001) has been deleted
https://codereview.chromium.org/2222583002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2222583002/diff/200001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:204: <div id="error-message" class$="[getErrorDirection_]"> On 2016/08/22 20:56:39, jdufault wrote: > [[getErrorDirection_()]] Done. https://codereview.chromium.org/2222583002/diff/200001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:205: [[errorMessage]] On 2016/08/22 20:56:40, jdufault wrote: > Please also add support for warnings. I'd rename errorMessage to problemMessage_ > and have a problemClass_='warning'/'error' Done. https://codereview.chromium.org/2222583002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2222583002/diff/200001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:194: * Computes the direction of the errorMessage. On 2016/08/22 20:56:40, jdufault wrote: > nit: error message Done. https://codereview.chromium.org/2222583002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/setup_pin_dialog.js (right): https://codereview.chromium.org/2222583002/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/setup_pin_dialog.js:157: this.pinKeyboard.errorMessage = this.i18n(messageId); On 2016/08/22 20:56:40, jdufault wrote: > what about this for a public API? > > this.pinKeyboard.setProblem(this.i18n(messageId), true /*isError*/); Done. https://codereview.chromium.org/2222583002/diff/200001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/2222583002/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:381: builder->Add("pinKeyboardErrorMessage", IDS_PIN_KEYBOARD_ERROR_MESSAGE); On 2016/08/22 20:56:40, jdufault wrote: > Move string to oobe strings, rename it so it is specific to a failed login. I removed this because this is not used anymore. https://codereview.chromium.org/2222583002/diff/200001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc (right): https://codereview.chromium.org/2222583002/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:876: {"pinKeyboardErrorMessage", IDS_PIN_KEYBOARD_ERROR_MESSAGE}, On 2016/08/22 20:56:40, jdufault wrote: > Remove this after the change above. Done.
https://codereview.chromium.org/2222583002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2222583002/diff/240001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:121: color: var(--google-red-700); I don't think if the problem is a warning message we want it to be in red. Have you talked to sgabriel@? https://codereview.chromium.org/2222583002/diff/240001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:197: [[getProblemClass_(problemMessage)]]" Can we use problemClass_ directly? https://codereview.chromium.org/2222583002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2222583002/diff/240001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:53: problemMessage: { This should be private (problemMessage_) https://codereview.chromium.org/2222583002/diff/240001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:61: problemClass: { This should be private (problemClass_) https://codereview.chromium.org/2222583002/diff/240001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:207: getProblemClass_: function(problemMessage) { Showing the problem should be controlled by the embedder, since the logic to do that varies. https://codereview.chromium.org/2222583002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/setup_pin_dialog.js (right): https://codereview.chromium.org/2222583002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/setup_pin_dialog.js:157: this.pinKeyboard.setProblem(this.i18n(messageId), true /*isError*/); Refactor the callers of showProblem_ to also use true/false. problemClass is not being propagated right now which breaks all of the warnings. If we want to eliminate the difference between warning and errors, we should eliminate the problemClass parameter entirely. https://codereview.chromium.org/2222583002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/setup_pin_dialog.js:163: this.pinKeyboard.setProblem('', true /*isError*/); Add a clearProblem method?
As mentioned earlier, sgabriel@ is removing the problem message from the pin keyboard, at least for the time being. https://codereview.chromium.org/2222583002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2222583002/diff/240001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:121: color: var(--google-red-700); On 2016/08/24 01:20:18, jdufault wrote: > I don't think if the problem is a warning message we want it to be in red. Have > you talked to sgabriel@? No more problem message. https://codereview.chromium.org/2222583002/diff/240001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:197: [[getProblemClass_(problemMessage)]]" On 2016/08/24 01:20:18, jdufault wrote: > Can we use problemClass_ directly? No more problem message. https://codereview.chromium.org/2222583002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2222583002/diff/240001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:53: problemMessage: { On 2016/08/24 01:20:18, jdufault wrote: > This should be private (problemMessage_) No more problem message. https://codereview.chromium.org/2222583002/diff/240001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:61: problemClass: { On 2016/08/24 01:20:18, jdufault wrote: > This should be private (problemClass_) No more problem message. https://codereview.chromium.org/2222583002/diff/240001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:207: getProblemClass_: function(problemMessage) { On 2016/08/24 01:20:18, jdufault wrote: > Showing the problem should be controlled by the embedder, since the logic to do > that varies. No more problem message. https://codereview.chromium.org/2222583002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/setup_pin_dialog.js (right): https://codereview.chromium.org/2222583002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/setup_pin_dialog.js:157: this.pinKeyboard.setProblem(this.i18n(messageId), true /*isError*/); On 2016/08/24 01:20:18, jdufault wrote: > Refactor the callers of showProblem_ to also use true/false. > > problemClass is not being propagated right now which breaks all of the warnings. > If we want to eliminate the difference between warning and errors, we should > eliminate the problemClass parameter entirely. No more problem message. https://codereview.chromium.org/2222583002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/setup_pin_dialog.js:163: this.pinKeyboard.setProblem('', true /*isError*/); On 2016/08/24 01:20:18, jdufault wrote: > Add a clearProblem method? No more problem message.
https://codereview.chromium.org/2222583002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2222583002/diff/280001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:182: [[getProblemClass_(problemMessage)]]" Remove? https://codereview.chromium.org/2222583002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2222583002/diff/280001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:135: return value.length > 0 ? 'has-content' : ''; Can these type of toggles be done by using an attribute binding? html: <foo has-content=[[hasContent(value)]]> css: foo[has-content] {}
Patchset #10 (id:300001) has been deleted
Patchset #10 (id:320001) has been deleted
https://codereview.chromium.org/2222583002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2222583002/diff/280001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:182: [[getProblemClass_(problemMessage)]]" On 2016/08/25 23:57:26, jdufault wrote: > Remove? Done. https://codereview.chromium.org/2222583002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2222583002/diff/280001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:135: return value.length > 0 ? 'has-content' : ''; On 2016/08/25 23:57:26, jdufault wrote: > Can these type of toggles be done by using an attribute binding? > > html: > > <foo has-content=[[hasContent(value)]]> > > css: > foo[has-content] {} Done.
https://codereview.chromium.org/2222583002/diff/340001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2222583002/diff/340001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:53: hasContent: { It doesn't look like this property ever gets updated. Either way, use a function instead of a property so you can declare the dependency inside of the html. Can you also apply this treatment to all of the other functions which follow the pattern function foo(a) { return a ? 'class' : ''; }
https://codereview.chromium.org/2222583002/diff/340001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2222583002/diff/340001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:53: hasContent: { On 2016/08/26 01:24:12, jdufault wrote: > It doesn't look like this property ever gets updated. Either way, use a function > instead of a property so you can declare the dependency inside of the html. > > Can you also apply this treatment to all of the other functions which follow the > pattern > > function foo(a) { > return a ? 'class' : ''; > } Done.
lgtm https://codereview.chromium.org/2222583002/diff/360001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2222583002/diff/360001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:28: </defs> Undo indent changes
xiyuan@ - Please take a look. Thanks!
lgtm https://codereview.chromium.org/2222583002/diff/360001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2222583002/diff/360001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:63: .digit-button[hidden] { nit: Should we use [hidden=true]? Would it work as expected when hidden is set to false? The worry is that [hidden] would select [hidden=false].
https://codereview.chromium.org/2222583002/diff/360001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2222583002/diff/360001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:63: .digit-button[hidden] { On 2016/08/26 21:11:31, xiyuan wrote: > nit: Should we use [hidden=true]? Would it work as expected when hidden is set > to false? The worry is that [hidden] would select [hidden=false]. +1, also pull it away from just .digit-button, ie, https://cs.chromium.org/chromium/src/ui/login/oobe.css?l=133
https://codereview.chromium.org/2222583002/diff/360001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2222583002/diff/360001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:28: </defs> On 2016/08/26 19:59:18, jdufault wrote: > Undo indent changes Done. https://codereview.chromium.org/2222583002/diff/360001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:63: .digit-button[hidden] { On 2016/08/26 21:11:31, xiyuan wrote: > nit: Should we use [hidden=true]? Would it work as expected when hidden is set > to false? The worry is that [hidden] would select [hidden=false]. Done. https://codereview.chromium.org/2222583002/diff/360001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:63: .digit-button[hidden] { On 2016/08/26 21:25:29, jdufault wrote: > On 2016/08/26 21:11:31, xiyuan wrote: > > nit: Should we use [hidden=true]? Would it work as expected when hidden is set > > to false? The worry is that [hidden] would select [hidden=false]. > > +1, also pull it away from just .digit-button, ie, > https://cs.chromium.org/chromium/src/ui/login/oobe.css?l=133 Done.
The CQ bit was checked by sammiequon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, jdufault@chromium.org Link to the patchset: https://codereview.chromium.org/2222583002/#ps380001 (title: "Nits.")
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: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by sammiequon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, jdufault@chromium.org Link to the patchset: https://codereview.chromium.org/2222583002/#ps400001 (title: "Fix for closure.")
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 ========== Lock screen pin keyboard ui upgrades. Changed the pin keyboard to match the specs at https://folio.googleplex.com/cros-core-ui/spec#%2F5.1%20-%20Start%20screen%20..., minus the submit button on the normal pod and the errors on the pin keyboard. BUG=627222 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Lock screen pin keyboard ui upgrades. Changed the pin keyboard to match the specs at https://folio.googleplex.com/cros-core-ui/spec#%2F5.1%20-%20Start%20screen%20..., minus the submit button on the normal pod and the errors on the pin keyboard. BUG=627222 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #13 (id:400001)
Message was sent while issue was closed.
Description was changed from ========== Lock screen pin keyboard ui upgrades. Changed the pin keyboard to match the specs at https://folio.googleplex.com/cros-core-ui/spec#%2F5.1%20-%20Start%20screen%20..., minus the submit button on the normal pod and the errors on the pin keyboard. BUG=627222 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Lock screen pin keyboard ui upgrades. Changed the pin keyboard to match the specs at https://folio.googleplex.com/cros-core-ui/spec#%2F5.1%20-%20Start%20screen%20..., minus the submit button on the normal pod and the errors on the pin keyboard. BUG=627222 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/12bda3871f8b16b531430d0a7db60ba4468d6c11 Cr-Commit-Position: refs/heads/master@{#414858} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/12bda3871f8b16b531430d0a7db60ba4468d6c11 Cr-Commit-Position: refs/heads/master@{#414858} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
