Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2467783002:
ChromeOS: update alignment of user POD error bubble. (Closed)
|
Created:
4 years, 1 month ago by Alexander Alekseev Modified:
4 years, 1 month ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChromeOS: update alignment of user POD error bubble.
BUG=642400
Committed: https://crrev.com/a02f7092cdd2bef11e1697732f61d3d3e1b10afd
Cr-Commit-Position: refs/heads/master@{#429752}
Patch Set 1 #Patch Set 2 : Removed some debug #
Total comments: 1
Patch Set 3 : Fixed calculation of bubble size #Patch Set 4 : Implemented side positioning. Removed debug. #
Total comments: 16
Patch Set 5 : Update after review. Also fixed CSS styles. #
Total comments: 6
Patch Set 6 : Update after review #
Total comments: 9
Patch Set 7 : Update after review. #Patch Set 8 : Bugfix. #
Messages
Total messages: 26 (9 generated)
alemate@chromium.org changed reviewers: + xiyuan@chromium.org
Xiyuan, what do you think about this? I could not find better implementation.
jdufault@chromium.org changed reviewers: + jdufault@chromium.org
https://codereview.chromium.org/2467783002/diff/20001/ui/login/account_picker... File ui/login/account_picker/screen_account_picker.js (right): https://codereview.chromium.org/2467783002/diff/20001/ui/login/account_picker... ui/login/account_picker/screen_account_picker.js:187: var bubbleAnchor = activatedPod; This works as expected when PIN is enabled? PIN greatly expands pod height.
Calculating layout via JS is fine since the error bubble and its anchor pod do not have parent/children relationship in DOM. jdufault@ is right that we need to handle the PIN case. BTW, the UX spec shows the error bubble on the right for PIN case and it seems not consistent with our code.
On 2016/11/01 17:38:53, xiyuan wrote: > jdufault@ is right that we need to handle the PIN case. BTW, the UX spec shows > the error bubble on the right for PIN case and it seems not consistent with our > code. I think the spec for the PIN warning was created for this bug; the behavior is different than originally described w/o the spec.
PTAL. This is still unfinished, but I fixed the check that bubble does not overlap the shelf. (Bubble side placement has not been implemented yet.)
Everything should work (except may be text/link/icon styling). PTAL.
The CQ bit was checked by alemate@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.
https://codereview.chromium.org/2467783002/diff/60001/ui/login/account_picker... File ui/login/account_picker/screen_account_picker.js (right): https://codereview.chromium.org/2467783002/diff/60001/ui/login/account_picker... ui/login/account_picker/screen_account_picker.js:218: var bubbleDisplay = bubble.style.display; I don't think bubble changes (or should change) its visibility and display so caching and restoring these two are not necessary. https://codereview.chromium.org/2467783002/diff/60001/ui/login/account_picker... ui/login/account_picker/screen_account_picker.js:222: bubble.classList.remove('faded'); "faded" controls the opacity and should not affect the layout. Could you try to see if we get the correct offsetWidth/offsetHeight without removing it? https://codereview.chromium.org/2467783002/diff/60001/ui/login/account_picker... ui/login/account_picker/screen_account_picker.js:247: } The code block to calculate size and choose the arrow position should be part of bubble. How about move it to showContentForElement in bubble.js ? https://codereview.chromium.org/2467783002/diff/60001/ui/login/bubble.js File ui/login/bubble.js (right): https://codereview.chromium.org/2467783002/diff/60001/ui/login/bubble.js#newc... ui/login/bubble.js:250: if (opt_match_width) { Would does match_width mean when bubble is on the left/right side of the anchor element? Do we still need to match width? Or should we match height? Or do nothing? https://codereview.chromium.org/2467783002/diff/60001/ui/login/bubble.js#newc... ui/login/bubble.js:252: var el_width = el_width -> elWidth, JS uses lowerThenCamelCase style https://codereview.chromium.org/2467783002/diff/60001/ui/login/bubble.js#newc... ui/login/bubble.js:254: var padding_left = parseInt(window.getComputedStyle(this, null) padding_left -> paddingLeft https://codereview.chromium.org/2467783002/diff/60001/ui/login/bubble.js#newc... ui/login/bubble.js:256: var padding_right = parseInt(window.getComputedStyle(this, null) padding_right -> paddingRight https://codereview.chromium.org/2467783002/diff/60001/ui/login/login_ui_tools.js File ui/login/login_ui_tools.js (right): https://codereview.chromium.org/2467783002/diff/60001/ui/login/login_ui_tools... ui/login/login_ui_tools.js:24: parseInt(window.getComputedStyle($('outer-container')).bottom); unrelated to your change, but could you wrap this with {} since you are here.
https://codereview.chromium.org/2467783002/diff/60001/ui/login/account_picker... File ui/login/account_picker/screen_account_picker.js (right): https://codereview.chromium.org/2467783002/diff/60001/ui/login/account_picker... ui/login/account_picker/screen_account_picker.js:218: var bubbleDisplay = bubble.style.display; On 2016/11/02 16:17:21, xiyuan wrote: > I don't think bubble changes (or should change) its visibility and display so > caching and restoring these two are not necessary. Done. https://codereview.chromium.org/2467783002/diff/60001/ui/login/account_picker... ui/login/account_picker/screen_account_picker.js:222: bubble.classList.remove('faded'); On 2016/11/02 16:17:21, xiyuan wrote: > "faded" controls the opacity and should not affect the layout. Could you try to > see if we get the correct offsetWidth/offsetHeight without removing it? Done. https://codereview.chromium.org/2467783002/diff/60001/ui/login/account_picker... ui/login/account_picker/screen_account_picker.js:247: } On 2016/11/02 16:17:21, xiyuan wrote: > The code block to calculate size and choose the arrow position should be part of > bubble. How about move it to showContentForElement in bubble.js ? Leaving it here as we decided that implementing asymmetric offsets in bubble.js is unreasonable. https://codereview.chromium.org/2467783002/diff/60001/ui/login/bubble.js File ui/login/bubble.js (right): https://codereview.chromium.org/2467783002/diff/60001/ui/login/bubble.js#newc... ui/login/bubble.js:250: if (opt_match_width) { On 2016/11/02 16:17:22, xiyuan wrote: > Would does match_width mean when bubble is on the left/right side of the anchor > element? Do we still need to match width? Or should we match height? Or do > nothing? From design mocks I guess that yes, we should use the same width. https://codereview.chromium.org/2467783002/diff/60001/ui/login/bubble.js#newc... ui/login/bubble.js:252: var el_width = On 2016/11/02 16:17:21, xiyuan wrote: > el_width -> elWidth, JS uses lowerThenCamelCase style Done. https://codereview.chromium.org/2467783002/diff/60001/ui/login/bubble.js#newc... ui/login/bubble.js:254: var padding_left = parseInt(window.getComputedStyle(this, null) On 2016/11/02 16:17:22, xiyuan wrote: > padding_left -> paddingLeft Done. https://codereview.chromium.org/2467783002/diff/60001/ui/login/bubble.js#newc... ui/login/bubble.js:256: var padding_right = parseInt(window.getComputedStyle(this, null) On 2016/11/02 16:17:21, xiyuan wrote: > padding_right -> paddingRight Done. https://codereview.chromium.org/2467783002/diff/60001/ui/login/login_ui_tools.js File ui/login/login_ui_tools.js (right): https://codereview.chromium.org/2467783002/diff/60001/ui/login/login_ui_tools... ui/login/login_ui_tools.js:24: parseInt(window.getComputedStyle($('outer-container')).bottom); On 2016/11/02 16:17:22, xiyuan wrote: > unrelated to your change, but could you wrap this with {} since you are here. Done.
lgtm https://codereview.chromium.org/2467783002/diff/80001/ui/login/account_picker... File ui/login/account_picker/screen_account_picker.js (right): https://codereview.chromium.org/2467783002/diff/80001/ui/login/account_picker... ui/login/account_picker/screen_account_picker.js:198: position = cr.ui.Bubble.Attachment.RIGHT; nit: position -> attachment (or arrowPosition) "position" is bit confusing. I think of coordinates (x,y) when I read it. https://codereview.chromium.org/2467783002/diff/80001/ui/login/account_picker... ui/login/account_picker/screen_account_picker.js:217: var bubbleHidden = bubble.hidden; nit: we don't need to cache bubble.hidden then restore it since we change its content and don't want to show it afterwards. That is, we should always do bubble.hidden = true in line 228 thus no need to have |bubbleHidden|. https://codereview.chromium.org/2467783002/diff/80001/ui/login/bubble.css File ui/login/bubble.css (right): https://codereview.chromium.org/2467783002/diff/80001/ui/login/bubble.css#new... ui/login/bubble.css:33: width: 100%; nit: can we use a fixed width, e.g. 20px ?
https://codereview.chromium.org/2467783002/diff/80001/ui/login/account_picker... File ui/login/account_picker/screen_account_picker.js (right): https://codereview.chromium.org/2467783002/diff/80001/ui/login/account_picker... ui/login/account_picker/screen_account_picker.js:198: position = cr.ui.Bubble.Attachment.RIGHT; On 2016/11/03 16:54:40, xiyuan wrote: > nit: position -> attachment (or arrowPosition) > > "position" is bit confusing. I think of coordinates (x,y) when I read it. Done. https://codereview.chromium.org/2467783002/diff/80001/ui/login/account_picker... ui/login/account_picker/screen_account_picker.js:217: var bubbleHidden = bubble.hidden; On 2016/11/03 16:54:40, xiyuan wrote: > nit: we don't need to cache bubble.hidden then restore it since we change its > content and don't want to show it afterwards. That is, we should always do > bubble.hidden = true in line 228 thus no need to have |bubbleHidden|. Done. https://codereview.chromium.org/2467783002/diff/80001/ui/login/bubble.css File ui/login/bubble.css (right): https://codereview.chromium.org/2467783002/diff/80001/ui/login/bubble.css#new... ui/login/bubble.css:33: width: 100%; On 2016/11/03 16:54:40, xiyuan wrote: > nit: can we use a fixed width, e.g. 20px ? Done.
lgtm Thanks.
Just some minor nits, lgtm https://codereview.chromium.org/2467783002/diff/100001/ui/login/account_picke... File ui/login/account_picker/screen_account_picker.js (right): https://codereview.chromium.org/2467783002/diff/100001/ui/login/account_picke... ui/login/account_picker/screen_account_picker.js:191: var elements = activatedPod.getElementsByClassName('auth-container'); Are there ever multiple auth-container elements on a pod? bubbleAnchor = activatePod.getElementByClassName('auth-container'); if (!bubbleAnchor) { /* error */ } // Is it even worth checking for this scenario? https://codereview.chromium.org/2467783002/diff/100001/ui/login/account_picke... ui/login/account_picker/screen_account_picker.js:207: // the element. Please describe why. https://codereview.chromium.org/2467783002/diff/100001/ui/login/account_picke... ui/login/account_picker/screen_account_picker.js:208: // 4 = bubble distance from element Make 4 a constant at the top of the file? https://codereview.chromium.org/2467783002/diff/100001/ui/login/bubble.js File ui/login/bubble.js (right): https://codereview.chromium.org/2467783002/diff/100001/ui/login/bubble.js#new... ui/login/bubble.js:194: opt_offset, opt_padding, opt_match_width) { Update doc
https://codereview.chromium.org/2467783002/diff/100001/ui/login/account_picke... File ui/login/account_picker/screen_account_picker.js (right): https://codereview.chromium.org/2467783002/diff/100001/ui/login/account_picke... ui/login/account_picker/screen_account_picker.js:191: var elements = activatedPod.getElementsByClassName('auth-container'); On 2016/11/03 23:37:03, jdufault wrote: > Are there ever multiple auth-container elements on a pod? > > bubbleAnchor = activatePod.getElementByClassName('auth-container'); > if (!bubbleAnchor) { /* error */ } // Is it even worth checking for this > scenario? > This is just in case I missed some specific use case here. Done. https://codereview.chromium.org/2467783002/diff/100001/ui/login/account_picke... ui/login/account_picker/screen_account_picker.js:207: // the element. On 2016/11/03 23:37:03, jdufault wrote: > Please describe why. Done. https://codereview.chromium.org/2467783002/diff/100001/ui/login/account_picke... ui/login/account_picker/screen_account_picker.js:208: // 4 = bubble distance from element On 2016/11/03 23:37:03, jdufault wrote: > Make 4 a constant at the top of the file? Done. https://codereview.chromium.org/2467783002/diff/100001/ui/login/bubble.js File ui/login/bubble.js (right): https://codereview.chromium.org/2467783002/diff/100001/ui/login/bubble.js#new... ui/login/bubble.js:194: opt_offset, opt_padding, opt_match_width) { On 2016/11/03 23:37:04, jdufault wrote: > Update doc Done.
https://codereview.chromium.org/2467783002/diff/100001/ui/login/account_picke... File ui/login/account_picker/screen_account_picker.js (right): https://codereview.chromium.org/2467783002/diff/100001/ui/login/account_picke... ui/login/account_picker/screen_account_picker.js:191: var elements = activatedPod.getElementsByClassName('auth-container'); On 2016/11/03 23:37:03, jdufault wrote: > Are there ever multiple auth-container elements on a pod? > > bubbleAnchor = activatePod.getElementByClassName('auth-container'); > if (!bubbleAnchor) { /* error */ } // Is it even worth checking for this > scenario? > > There is no "getElementByClassName" .
The CQ bit was checked by alemate@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/2467783002/#ps140001 (title: "Bugfix.")
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.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== ChromeOS: update alignment of user POD error bubble. BUG=642400 ========== to ========== ChromeOS: update alignment of user POD error bubble. BUG=642400 Committed: https://crrev.com/a02f7092cdd2bef11e1697732f61d3d3e1b10afd Cr-Commit-Position: refs/heads/master@{#429752} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/a02f7092cdd2bef11e1697732f61d3d3e1b10afd Cr-Commit-Position: refs/heads/master@{#429752} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
