Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2675773002:
VR menu mode styling basics. (Closed)
DescriptionVR menu mode styling basics.
- Shrink the content quad in menu mode, and add transparency.
- Position the buttons more like UX proposals.
- Next the omnibox in with buttons.
BUG=644565
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2675773002
Cr-Commit-Position: refs/heads/master@{#448359}
Committed: https://chromium.googlesource.com/chromium/src/+/555c81dca33110db593ac5c0951579a372f6878b
Patch Set 1 #
Total comments: 9
Patch Set 2 : Rework toward Biao's suggestions. #
Total comments: 4
Patch Set 3 : Closure compiler appeasement. #Patch Set 4 : Rebase to ToT. #Messages
Total messages: 21 (9 generated)
Description was changed from ========== VR menu mode styling basics. - Shrink the content quad in menu mode, and add transparency. - Position the buttons more like UX proposals. - Next the omnibox in with buttons. BUG=644565 ========== to ========== VR menu mode styling basics. - Shrink the content quad in menu mode, and add transparency. - Position the buttons more like UX proposals. - Next the omnibox in with buttons. BUG=644565 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
cjgrant@chromium.org changed reviewers: + bshe@chromium.org, mthiesse@chromium.org, tiborg@google.com
https://codereview.chromium.org/2675773002/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): https://codereview.chromium.org/2675773002/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:56: setMode(fullscreen, menuMode) { you can probably define all variables outside if statement. Also, it feels like that mode should be an enum? i.e. If menuMode is true, fullscreen will be ignored. It is not very obvious from this function currently. https://codereview.chromium.org/2675773002/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:60: var z = -1.2; nit: avoid magic number.
https://codereview.chromium.org/2675773002/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): https://codereview.chromium.org/2675773002/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:56: setMode(fullscreen, menuMode) { On 2017/02/03 16:53:12, bshe wrote: > you can probably define all variables outside if statement. > Also, it feels like that mode should be an enum? i.e. If menuMode is true, > fullscreen will be ignored. It is not very obvious from this function currently. Agreed that the naming could be better. Some of the other elements work by offering: setFullScreen(bool) setMenuMode(bool) And then choose behaviour based on the two states. But, I don't think mode should be an enum, as "fullscreen" and "menu showing" are independent concepts. I'll rework this. https://codereview.chromium.org/2675773002/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:60: var z = -1.2; On 2017/02/03 16:53:12, bshe wrote: > nit: avoid magic number. So here, the numbers really are a bit magic. One improvement could be better variable names: content_y_position = 0.1; Or, as we did elsewhere, have this.CONTENT_Y_POSITION_IN_MENU_MODE = 0.1, which doesn't seem to help readability that much. Let me know what you think..
https://codereview.chromium.org/2675773002/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): https://codereview.chromium.org/2675773002/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:56: setMode(fullscreen, menuMode) { On 2017/02/03 20:38:11, cjgrant wrote: > On 2017/02/03 16:53:12, bshe wrote: > > you can probably define all variables outside if statement. > > Also, it feels like that mode should be an enum? i.e. If menuMode is true, > > fullscreen will be ignored. It is not very obvious from this function > currently. > > Agreed that the naming could be better. Some of the other elements work by > offering: > setFullScreen(bool) > setMenuMode(bool) > > And then choose behaviour based on the two states. But, I don't think mode > should be an enum, as "fullscreen" and "menu showing" are independent concepts. > I'll rework this. Agree that enum is probably not a good idea. IIUC, our UI current have normal, fullscreen and webvr state. And in each state, it could enter menu mode. Exit menu mode should continue previous state. So make menu mode independent of other states would be nice. If it turns out to be large CL. I am fine with landing this one and then do it in a follow up. https://codereview.chromium.org/2675773002/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:60: var z = -1.2; On 2017/02/03 20:38:11, cjgrant wrote: > On 2017/02/03 16:53:12, bshe wrote: > > nit: avoid magic number. > > So here, the numbers really are a bit magic. One improvement could be better > variable names: > > content_y_position = 0.1; > > Or, as we did elsewhere, have this.CONTENT_Y_POSITION_IN_MENU_MODE = 0.1, which > doesn't seem to help readability that much. Let me know what you think.. I remember the coding style requires avoiding magic number as much as possible. Make it a constant would also help us avoid forget to update the value in one of the places that it is used. So I would prefer to use a constant name here even though it doesn't improve readability much...
https://codereview.chromium.org/2675773002/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): https://codereview.chromium.org/2675773002/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:50: setOpacity(opacity) { I got rid of this as it was for demo purposes rather than UX spec. https://codereview.chromium.org/2675773002/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:56: setMode(fullscreen, menuMode) { On 2017/02/03 21:36:41, bshe wrote: > On 2017/02/03 20:38:11, cjgrant wrote: > > On 2017/02/03 16:53:12, bshe wrote: > > > you can probably define all variables outside if statement. > > > Also, it feels like that mode should be an enum? i.e. If menuMode is true, > > > fullscreen will be ignored. It is not very obvious from this function > > currently. > > > > Agreed that the naming could be better. Some of the other elements work by > > offering: > > setFullScreen(bool) > > setMenuMode(bool) > > > > And then choose behaviour based on the two states. But, I don't think mode > > should be an enum, as "fullscreen" and "menu showing" are independent > concepts. > > I'll rework this. > > Agree that enum is probably not a good idea. IIUC, our UI current have normal, > fullscreen and > webvr state. And in each state, it could enter menu mode. Exit menu mode should > continue > previous state. So make menu mode independent of other states would be nice. If > it turns out > to be large CL. I am fine with landing this one and then do it in a follow up. Let me know what you think of the latest patch set. It's more consistent with how the other UI elements work - that is, give ContentQuad visibility of all parameters independently, and let it choose how to look based on each. https://codereview.chromium.org/2675773002/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:60: var z = -1.2; On 2017/02/03 21:36:41, bshe wrote: > On 2017/02/03 20:38:11, cjgrant wrote: > > On 2017/02/03 16:53:12, bshe wrote: > > > nit: avoid magic number. > > > > So here, the numbers really are a bit magic. One improvement could be better > > variable names: > > > > content_y_position = 0.1; > > > > Or, as we did elsewhere, have this.CONTENT_Y_POSITION_IN_MENU_MODE = 0.1, > which > > doesn't seem to help readability that much. Let me know what you think.. > > I remember the coding style requires avoiding magic number as much as possible. > Make it a constant would also help us avoid forget to update the value in one of > the places that it > is used. So I would prefer to use a constant name here even though it doesn't > improve readability much... Done. No more magic numbers other than 0. I also use defaults to make it more obviou which properties change in each mode.
On 2017/02/03 23:13:10, cjgrant wrote: > https://codereview.chromium.org/2675773002/diff/1/chrome/browser/resources/vr... > File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): > > https://codereview.chromium.org/2675773002/diff/1/chrome/browser/resources/vr... > chrome/browser/resources/vr_shell/vr_shell_ui.js:50: setOpacity(opacity) { > I got rid of this as it was for demo purposes rather than UX spec. > > https://codereview.chromium.org/2675773002/diff/1/chrome/browser/resources/vr... > chrome/browser/resources/vr_shell/vr_shell_ui.js:56: setMode(fullscreen, > menuMode) { > On 2017/02/03 21:36:41, bshe wrote: > > On 2017/02/03 20:38:11, cjgrant wrote: > > > On 2017/02/03 16:53:12, bshe wrote: > > > > you can probably define all variables outside if statement. > > > > Also, it feels like that mode should be an enum? i.e. If menuMode is true, > > > > fullscreen will be ignored. It is not very obvious from this function > > > currently. > > > > > > Agreed that the naming could be better. Some of the other elements work by > > > offering: > > > setFullScreen(bool) > > > setMenuMode(bool) > > > > > > And then choose behaviour based on the two states. But, I don't think mode > > > should be an enum, as "fullscreen" and "menu showing" are independent > > concepts. > > > I'll rework this. > > > > Agree that enum is probably not a good idea. IIUC, our UI current have normal, > > fullscreen and > > webvr state. And in each state, it could enter menu mode. Exit menu mode > should > > continue > > previous state. So make menu mode independent of other states would be nice. > If > > it turns out > > to be large CL. I am fine with landing this one and then do it in a follow up. > > Let me know what you think of the latest patch set. It's more consistent with > how the other UI elements work - that is, give ContentQuad visibility of all > parameters independently, and let it choose how to look based on each. > > https://codereview.chromium.org/2675773002/diff/1/chrome/browser/resources/vr... > chrome/browser/resources/vr_shell/vr_shell_ui.js:60: var z = -1.2; > On 2017/02/03 21:36:41, bshe wrote: > > On 2017/02/03 20:38:11, cjgrant wrote: > > > On 2017/02/03 16:53:12, bshe wrote: > > > > nit: avoid magic number. > > > > > > So here, the numbers really are a bit magic. One improvement could be > better > > > variable names: > > > > > > content_y_position = 0.1; > > > > > > Or, as we did elsewhere, have this.CONTENT_Y_POSITION_IN_MENU_MODE = 0.1, > > which > > > doesn't seem to help readability that much. Let me know what you think.. > > > > I remember the coding style requires avoiding magic number as much as > possible. > > Make it a constant would also help us avoid forget to update the value in one > of > > the places that it > > is used. So I would prefer to use a constant name here even though it doesn't > > improve readability much... > > Done. No more magic numbers other than 0. I also use defaults to make it more > obviou which properties change in each mode. lgtm
https://codereview.chromium.org/2675773002/diff/20001/chrome/browser/resource... File chrome/browser/resources/vr_shell/vr_shell_ui.html (right): https://codereview.chromium.org/2675773002/diff/20001/chrome/browser/resource... chrome/browser/resources/vr_shell/vr_shell_ui.html:50: <div id="suggestions"> Should the ID be called "omnibox-suggestions" since all omnibox element IDs start with omnibox? https://codereview.chromium.org/2675773002/diff/20001/chrome/browser/resource... File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): https://codereview.chromium.org/2675773002/diff/20001/chrome/browser/resource... chrome/browser/resources/vr_shell/vr_shell_ui.js:253: update.setRotation(1, 0, 0, -0.8); Should these numbers be defined as const? Maybe it's overkill since it's a dev button anyways.
https://codereview.chromium.org/2675773002/diff/20001/chrome/browser/resource... File chrome/browser/resources/vr_shell/vr_shell_ui.html (right): https://codereview.chromium.org/2675773002/diff/20001/chrome/browser/resource... chrome/browser/resources/vr_shell/vr_shell_ui.html:50: <div id="suggestions"> On 2017/02/06 16:05:45, tiborg wrote: > Should the ID be called "omnibox-suggestions" since all omnibox element IDs > start with omnibox? So I renamed from omnibox-suggestions to suggestions to shorten the name. The original prefixing was meant to avoid naming conflicts for very general sounding elements, but really, we should be query-selecting on the omnibox root element anyway for efficiency, so a name conflict wouldn't matter. https://codereview.chromium.org/2675773002/diff/20001/chrome/browser/resource... File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): https://codereview.chromium.org/2675773002/diff/20001/chrome/browser/resource... chrome/browser/resources/vr_shell/vr_shell_ui.js:253: update.setRotation(1, 0, 0, -0.8); On 2017/02/06 16:05:45, tiborg wrote: > Should these numbers be defined as const? Maybe it's overkill since it's a dev > button anyways. I didn't bother here, as it's a dev button, and its location/presentation change whenever they interfere with the UI. The Josh meetings this week should help us pick a better path for defining all such properties in CSS.
The CQ bit was checked by cjgrant@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bshe@chromium.org Link to the patchset: https://codereview.chromium.org/2675773002/#ps40001 (title: "Closure compiler appeasement.")
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by cjgrant@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bshe@chromium.org Link to the patchset: https://codereview.chromium.org/2675773002/#ps60001 (title: "Rebase to ToT.")
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": 60001, "attempt_start_ts": 1486405604823540,
"parent_rev": "5fcf9646d2502ef96648cbefb180af1c1f339984", "commit_rev":
"555c81dca33110db593ac5c0951579a372f6878b"}
Message was sent while issue was closed.
Description was changed from ========== VR menu mode styling basics. - Shrink the content quad in menu mode, and add transparency. - Position the buttons more like UX proposals. - Next the omnibox in with buttons. BUG=644565 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== VR menu mode styling basics. - Shrink the content quad in menu mode, and add transparency. - Position the buttons more like UX proposals. - Next the omnibox in with buttons. BUG=644565 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2675773002 Cr-Commit-Position: refs/heads/master@{#448359} Committed: https://chromium.googlesource.com/chromium/src/+/555c81dca33110db593ac5c09515... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/555c81dca33110db593ac5c09515... |
