Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2081873008:
Changed look of pin keyboard. (Closed)
|
Created:
4 years, 6 months ago by sammiequon Modified:
4 years, 6 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. |
DescriptionChanged look of pin keyboard.
https://screenshot.googleplex.com/UaKhr1x3vKp
BUG=612221
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/f67abbb4cc787728557fd688bb2afcda9b13d845
Cr-Commit-Position: refs/heads/master@{#401980}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Fixed patch set 1 errors. #
Total comments: 12
Patch Set 3 : Fixed patch set 2 errors. #
Total comments: 2
Patch Set 4 : Rebased. #
Messages
Total messages: 29 (13 generated)
Description was changed from ========== Changed look of pin keyboard. Changed look of pin keyboard. BUG=612221 ========== to ========== Changed look of pin keyboard. Changed look of pin keyboard. BUG=612221 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Changed look of pin keyboard. Changed look of pin keyboard. BUG=612221 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Changed look of pin keyboard. BUG=612221 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Changed look of pin keyboard. BUG=612221 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Changed look of pin keyboard. https://screenshot.googleplex.com/UaKhr1x3vKp BUG=612221 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
sammiequon@chromium.org changed reviewers: + jdufault@chromium.org
@jdufault, PTAL
Can you reparent this CL to the user-pod animation CL? That way, the PIN keyboard will be positioned correctly which will make it easier to align to the mocks. https://codereview.chromium.org/2081873008/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2081873008/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:19: #container-constrained-width { This entire block should be removed. https://codereview.chromium.org/2081873008/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:174: <div class="icon-div"> icon-button, icon-div, text-div can all be done using selectors, right? Or do they need to actually be separate classes? https://codereview.chromium.org/2081873008/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2081873008/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:23: pin.value = ''; Does this need to be special cased? ''.substring(0, -1) evals to '' https://codereview.chromium.org/2081873008/diff/1/ui/login/account_picker/use... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/2081873008/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.css:8: #pin-container { Is this css needed? Can some of it be removed if you reparent onto the user pod animation CL?
https://codereview.chromium.org/2081873008/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2081873008/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:19: #container-constrained-width { On 2016/06/22 18:03:43, jdufault wrote: > This entire block should be removed. Done. https://codereview.chromium.org/2081873008/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:174: <div class="icon-div"> On 2016/06/22 18:03:44, jdufault wrote: > icon-button, icon-div, text-div can all be done using selectors, right? Or do > they need to actually be separate classes? icon-div is used to highlight the background of the icon, but i removed the other two. https://codereview.chromium.org/2081873008/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:174: <div class="icon-div"> On 2016/06/22 18:03:44, jdufault wrote: > icon-button, icon-div, text-div can all be done using selectors, right? Or do > they need to actually be separate classes? Done. https://codereview.chromium.org/2081873008/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2081873008/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:23: pin.value = ''; On 2016/06/22 18:03:44, jdufault wrote: > Does this need to be special cased? > > ''.substring(0, -1) evals to '' Done. https://codereview.chromium.org/2081873008/diff/1/ui/login/account_picker/use... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/2081873008/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.css:8: #pin-container { On 2016/06/22 18:03:44, jdufault wrote: > Is this css needed? Can some of it be removed if you reparent onto the user pod > animation CL? Done.
https://codereview.chromium.org/2081873008/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2081873008/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:50: margin: auto; Is this needed? https://codereview.chromium.org/2081873008/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:56: display: block; Is display: block needed? https://codereview.chromium.org/2081873008/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:63: .digit-button .icon-text { Can you please verify that all of the CSS here contains the minimum that needs to be specified? https://codereview.chromium.org/2081873008/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:87: width: 75%; Can you eliminate the % and use a margin/offset instead? https://codereview.chromium.org/2081873008/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:157: <div class="icon-div"> Is there a better class name than icon-div and icon-text? icon-container? icon-subheading? https://codereview.chromium.org/2081873008/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2081873008/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:18: onPinClear_: function() { onPinBackspace_? Though that doesn't sound much better, either. Maybe just update the comment to call out that this erases the last character.
https://codereview.chromium.org/2081873008/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2081873008/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:50: margin: auto; On 2016/06/22 23:18:00, jdufault wrote: > Is this needed? Yes. https://codereview.chromium.org/2081873008/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:56: display: block; On 2016/06/22 23:18:00, jdufault wrote: > Is display: block needed? Yes. https://codereview.chromium.org/2081873008/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:63: .digit-button .icon-text { On 2016/06/22 23:18:01, jdufault wrote: > Can you please verify that all of the CSS here contains the minimum that needs > to be specified? Yes. https://codereview.chromium.org/2081873008/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:87: width: 75%; On 2016/06/22 23:18:00, jdufault wrote: > Can you eliminate the % and use a margin/offset instead? Done. https://codereview.chromium.org/2081873008/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:157: <div class="icon-div"> On 2016/06/22 23:18:00, jdufault wrote: > Is there a better class name than icon-div and icon-text? icon-container? > icon-subheading? Done. https://codereview.chromium.org/2081873008/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2081873008/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:18: onPinClear_: function() { On 2016/06/22 23:18:01, jdufault wrote: > onPinBackspace_? Though that doesn't sound much better, either. > > Maybe just update the comment to call out that this erases the last character. Done.
lgtm; this will most likely have to be rebased onto https://codereview.chromium.org/2084013003/.
sammiequon@chromium.org changed reviewers: + xiyuan@chromium.org
@xiyuan - Please take a look.
On 2016/06/23 18:45:13, sammiequon wrote: > @xiyuan - Please take a look. xiyuan@ - Please take a look.
https://codereview.chromium.org/2081873008/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2081873008/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:59: top: 4px; Is .icon refers to positioned element (e.g. has position:absoute)? If not, left/top is probably ignored and could be removed.
https://codereview.chromium.org/2081873008/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2081873008/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:59: top: 4px; On 2016/06/23 19:24:36, xiyuan wrote: > Is .icon refers to positioned element (e.g. has position:absoute)? If not, > left/top is probably ignored and could be removed. .icon is some overrides for iron-icon which is position:relative. I don't know if I did this correct but the position is way off without the left/top.
On 2016/06/23 19:49:32, sammiequon wrote: > https://codereview.chromium.org/2081873008/diff/40001/chrome/browser/resource... > File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): > > https://codereview.chromium.org/2081873008/diff/40001/chrome/browser/resource... > chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:59: top: 4px; > On 2016/06/23 19:24:36, xiyuan wrote: > > Is .icon refers to positioned element (e.g. has position:absoute)? If not, > > left/top is probably ignored and could be removed. > > .icon is some overrides for iron-icon which is position:relative. I don't know > if I did this correct but the position is way off without the left/top. position:relative makes it positioned so left/top would have effect. lgtm then
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) 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 xiyuan@chromium.org, jdufault@chromium.org Link to the patchset: https://codereview.chromium.org/2081873008/#ps100001 (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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
Message was sent while issue was closed.
Description was changed from ========== Changed look of pin keyboard. https://screenshot.googleplex.com/UaKhr1x3vKp BUG=612221 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Changed look of pin keyboard. https://screenshot.googleplex.com/UaKhr1x3vKp BUG=612221 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Changed look of pin keyboard. https://screenshot.googleplex.com/UaKhr1x3vKp BUG=612221 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Changed look of pin keyboard. https://screenshot.googleplex.com/UaKhr1x3vKp BUG=612221 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/f67abbb4cc787728557fd688bb2afcda9b13d845 Cr-Commit-Position: refs/heads/master@{#401980} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/f67abbb4cc787728557fd688bb2afcda9b13d845 Cr-Commit-Position: refs/heads/master@{#401980} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
