Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2516903002:
PS - Adjusting Public Session login pod for expanded whitelisting (Closed)
|
Created:
4 years, 1 month ago by Ivan Šandrk Modified:
4 years ago CC:
chromium-reviews, alemate+watch_chromium.org, achuith+watch_chromium.org, oshima+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPS - Adjusting Public Session login pod for expanded whitelisting
Approximate mocks of the new design https://folio.googleplex.com/cros-public-session-ui
BUG=667051
Committed: https://crrev.com/12f48eb6682abca50f059b65beebc31d49fad3f4
Cr-Commit-Position: refs/heads/master@{#434153}
Patch Set 1 #Patch Set 2 : Fixed monitoring-dialog position in advanced view #
Total comments: 17
Patch Set 3 : Fixed moved icon #Patch Set 4 : Nits; alpha sort, -webkit-margin-start #Patch Set 5 : Moved icon to left side #Patch Set 6 : Switched to cr.ui.dialogs.BaseDialog #Patch Set 7 : Disable background while the modal dialog is shown #Patch Set 8 : Fixed alpha sort #
Total comments: 1
Patch Set 9 : Added a prefix for CSS selectors to avoid clashes with other uses of BaseDialog #
Total comments: 6
Patch Set 10 : Cache container div, this.parentNode #
Messages
Total messages: 36 (18 generated)
isandrk@chromium.org changed reviewers: + achuith@chromium.org
Hey Achuith, please take a look at the changes :) Thanks, Ivan
achuith@chromium.org changed reviewers: + xiyuan@chromium.org
This looks ok to me. Xiyuan for a second set of eyes as my html isn't the best.
https://codereview.chromium.org/2516903002/diff/20001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/2516903002/diff/20001/ui/login/account_picker... ui/login/account_picker/user_pod_row.css:703: height: 230px; nit: alpha sort, i.e height should be before width See https://www.chromium.org/developers/web-development-style-guide, look for "Alphabetize properties." https://codereview.chromium.org/2516903002/diff/20001/ui/login/account_picker... ui/login/account_picker/user_pod_row.css:708: height: 280px; nit: alpha sort https://codereview.chromium.org/2516903002/diff/20001/ui/login/account_picker... ui/login/account_picker/user_pod_row.css:755: height: 263px; nit: alpha sort https://codereview.chromium.org/2516903002/diff/20001/ui/login/account_picker... ui/login/account_picker/user_pod_row.css:831: margin-bottom: 4px; nit: alpha sort https://codereview.chromium.org/2516903002/diff/20001/ui/login/account_picker... ui/login/account_picker/user_pod_row.css:878: z-index: 10; nit: alpha sort here https://codereview.chromium.org/2516903002/diff/20001/ui/login/account_picker... ui/login/account_picker/user_pod_row.css:883: margin-left: -160px; nit: -webkit-margin-start? xxx-left might not work with RTL https://codereview.chromium.org/2516903002/diff/20001/ui/login/account_picker... ui/login/account_picker/user_pod_row.css:890: margin-left: -115px; nit: similar here https://codereview.chromium.org/2516903002/diff/20001/ui/login/account_picker... File ui/login/account_picker/user_pod_template.html (right): https://codereview.chromium.org/2516903002/diff/20001/ui/login/account_picker... ui/login/account_picker/user_pod_template.html:176: <paper-dialog class="monitoring-dialog" role="dialog" tabindex="-1" Can we re-use existing dialogs (e.g. one of cr.ui.dialogs)? If not, is it possible to make it a generic dialog tool that other part of the login screen can use in the future? If that is also not feasible, could we move this out of the template DOM since we don't want a copy of this dialog for each cloned pod element.
The CQ bit was checked by isandrk@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...
https://codereview.chromium.org/2516903002/diff/20001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/2516903002/diff/20001/ui/login/account_picker... ui/login/account_picker/user_pod_row.css:703: height: 230px; On 2016/11/21 19:38:35, xiyuan (ooo Nov 28) wrote: > nit: alpha sort, i.e height should be before width > > See https://www.chromium.org/developers/web-development-style-guide, look for > "Alphabetize properties." Done. https://codereview.chromium.org/2516903002/diff/20001/ui/login/account_picker... ui/login/account_picker/user_pod_row.css:708: height: 280px; On 2016/11/21 19:38:35, xiyuan (ooo Nov 28) wrote: > nit: alpha sort Done. https://codereview.chromium.org/2516903002/diff/20001/ui/login/account_picker... ui/login/account_picker/user_pod_row.css:755: height: 263px; On 2016/11/21 19:38:35, xiyuan (ooo Nov 28) wrote: > nit: alpha sort Done. https://codereview.chromium.org/2516903002/diff/20001/ui/login/account_picker... ui/login/account_picker/user_pod_row.css:831: margin-bottom: 4px; On 2016/11/21 19:38:35, xiyuan (ooo Nov 28) wrote: > nit: alpha sort Done. https://codereview.chromium.org/2516903002/diff/20001/ui/login/account_picker... ui/login/account_picker/user_pod_row.css:878: z-index: 10; On 2016/11/21 19:38:35, xiyuan (ooo Nov 28) wrote: > nit: alpha sort here Done. https://codereview.chromium.org/2516903002/diff/20001/ui/login/account_picker... ui/login/account_picker/user_pod_row.css:883: margin-left: -160px; On 2016/11/21 19:38:35, xiyuan (ooo Nov 28) wrote: > nit: -webkit-margin-start? xxx-left might not work with RTL Done. https://codereview.chromium.org/2516903002/diff/20001/ui/login/account_picker... ui/login/account_picker/user_pod_row.css:890: margin-left: -115px; On 2016/11/21 19:38:35, xiyuan (ooo Nov 28) wrote: > nit: similar here Done. https://codereview.chromium.org/2516903002/diff/20001/ui/login/account_picker... File ui/login/account_picker/user_pod_template.html (right): https://codereview.chromium.org/2516903002/diff/20001/ui/login/account_picker... ui/login/account_picker/user_pod_template.html:176: <paper-dialog class="monitoring-dialog" role="dialog" tabindex="-1" On 2016/11/21 19:38:35, xiyuan (ooo Nov 28) wrote: > Can we re-use existing dialogs (e.g. one of cr.ui.dialogs)? > > If not, is it possible to make it a generic dialog tool that other part of the > login screen can use in the future? > > If that is also not feasible, could we move this out of the template DOM since > we don't want a copy of this dialog for each cloned pod element. Do you know of any existing dialogs that just have an exit button (on the top right)? No ok or cancel.
https://codereview.chromium.org/2516903002/diff/20001/ui/login/account_picker... File ui/login/account_picker/user_pod_template.html (right): https://codereview.chromium.org/2516903002/diff/20001/ui/login/account_picker... ui/login/account_picker/user_pod_template.html:176: <paper-dialog class="monitoring-dialog" role="dialog" tabindex="-1" On 2016/11/21 21:48:42, Ivan Šandrk wrote: > On 2016/11/21 19:38:35, xiyuan (ooo Nov 28) wrote: > > Can we re-use existing dialogs (e.g. one of cr.ui.dialogs)? > > > > If not, is it possible to make it a generic dialog tool that other part of the > > login screen can use in the future? > > > > If that is also not feasible, could we move this out of the template DOM since > > we don't want a copy of this dialog for each cloned pod element. > > Do you know of any existing dialogs that just have an exit button (on the top > right)? No ok or cancel. We can extend cr.ui.dialogs to support this. Or make css rules to hide the buttons.
The CQ bit was checked by isandrk@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...
I've switched over to cr.ui.dialogs.BaseDialog. Everything works as expected, except one thing. When the modal dialog is open, clicks outside it cause the user pod to change state (size). Here are the screenshots: - before click - https://drive.google.com/a/google.com/file/d/0B4cJVfvxbnGQODdzcGNuMDM4UlU/vie... - after click - https://drive.google.com/open?id=0B4cJVfvxbnGQbHBhX016MFZyWms Xiyuan, do you maybe know why I'm having problems with it?
Sorry, a click anywhere causes the POD to switch state (even inside the dialog itself, and even when closing the dialog via the close button).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by isandrk@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...
Xiyuan, now I've fixed it all, ready for reviewing :)
The CQ bit was checked by isandrk@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...
Mostly good. https://codereview.chromium.org/2516903002/diff/140001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/2516903002/diff/140001/ui/login/account_picke... ui/login/account_picker/user_pod_row.css:878: .cr-dialog-buttons { cr.ui.dialogs is used in a few other places in login webui [1]. Can we be more specific here to avoid affecting them? Think we might need to be more specific for other cr-dialog-xxx rules too. [1]: https://cs.chromium.org/search/?q=cr.ui.dialogs+file:/login/.*%5C.js&sq=packa...
On 2016/11/22 19:02:32, xiyuan (ooo Nov 28) wrote: > Mostly good. > > https://codereview.chromium.org/2516903002/diff/140001/ui/login/account_picke... > File ui/login/account_picker/user_pod_row.css (right): > > https://codereview.chromium.org/2516903002/diff/140001/ui/login/account_picke... > ui/login/account_picker/user_pod_row.css:878: .cr-dialog-buttons { > cr.ui.dialogs is used in a few other places in login webui [1]. Can we be more > specific here to avoid affecting them? Think we might need to be more specific > for other cr-dialog-xxx rules too. > > [1]: > https://cs.chromium.org/search/?q=cr.ui.dialogs+file:/login/.*%5C.js&sq=packa... Added a dummy container parent for additional CSS selectors prefix.
lgtm https://codereview.chromium.org/2516903002/diff/160001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2516903002/diff/160001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:2254: var dialogContainer = document.createElement('div'); nit: Cache the container div to avoid creating it every time when we show the dialog? https://codereview.chromium.org/2516903002/diff/160001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:2276: document.querySelector('#pod-row').disabled = true; nit: replace document.querySelector('#pod-row') with this.parentNode https://codereview.chromium.org/2516903002/diff/160001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:2283: document.querySelector('#pod-row').disabled = false; nit: document.querySelector('#pod-row') -> this.parentNode
The CQ bit was checked by isandrk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org Link to the patchset: https://codereview.chromium.org/2516903002/#ps180001 (title: "Cache container div, this.parentNode")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
All done, thank you for your help! https://codereview.chromium.org/2516903002/diff/160001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2516903002/diff/160001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:2254: var dialogContainer = document.createElement('div'); On 2016/11/23 00:23:07, xiyuan (ooo Nov 28) wrote: > nit: Cache the container div to avoid creating it every time when we show the > dialog? Done. https://codereview.chromium.org/2516903002/diff/160001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:2276: document.querySelector('#pod-row').disabled = true; On 2016/11/23 00:23:07, xiyuan (ooo Nov 28) wrote: > nit: replace > document.querySelector('#pod-row') > with > this.parentNode Done. https://codereview.chromium.org/2516903002/diff/160001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:2283: document.querySelector('#pod-row').disabled = false; On 2016/11/23 00:23:07, xiyuan (ooo Nov 28) wrote: > nit: document.querySelector('#pod-row') -> this.parentNode Done.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_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 isandrk@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": 180001, "attempt_start_ts": 1479902024399490,
"parent_rev": "e3a58ed702c7c00d2ebf2b28e92fc8e4abe17d99", "commit_rev":
"58f88693927662ffca0388ca28c58094a2318709"}
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== PS - Adjusting Public Session login pod for expanded whitelisting Approximate mocks of the new design https://folio.googleplex.com/cros-public-session-ui BUG=667051 ========== to ========== PS - Adjusting Public Session login pod for expanded whitelisting Approximate mocks of the new design https://folio.googleplex.com/cros-public-session-ui BUG=667051 Committed: https://crrev.com/12f48eb6682abca50f059b65beebc31d49fad3f4 Cr-Commit-Position: refs/heads/master@{#434153} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/12f48eb6682abca50f059b65beebc31d49fad3f4 Cr-Commit-Position: refs/heads/master@{#434153} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
