Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2670833005:
Improve VR omnibox styling and function (Closed)
DescriptionImprove VR omnibox styling and function
- Improve the spacing and alignment of suggestions.
- Clear suggestions when doing a navigation action.
- Tweak the "Reload UI" debugging button to be more usable.
BUG=641508
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2670833005
Cr-Commit-Position: refs/heads/master@{#448291}
Committed: https://chromium.googlesource.com/chromium/src/+/db713b182a14c3aacabed95d3b28f8885b1a633f
Patch Set 1 #
Total comments: 11
Patch Set 2 : Add CSS comments to unintuitive properties, as per Biao's suggestion. #Patch Set 3 : Fix closure compiler failure. Yay closure compiler. #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 20 (9 generated)
Description was changed from ========== Improve VR omnibox styling and function - Improve the spacing and alignment of suggestions. - Clear suggestions when doing a navigation action. - Tweak the "Reload UI" debugging button to be more usable. BUG=641508 ========== to ========== Improve VR omnibox styling and function - Improve the spacing and alignment of suggestions. - Clear suggestions when doing a navigation action. - Tweak the "Reload UI" debugging button to be more usable. BUG=641508 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/2670833005/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui.css (right): https://codereview.chromium.org/2670833005/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.css:212: align-items: center; I am wondering what's the element that you want to center? It doesn't look like suggestion has any child element? https://codereview.chromium.org/2670833005/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.css:215: border-right: 5px solid transparent; I am not sure why do you need transparent border here. Can you add comment about it? https://codereview.chromium.org/2670833005/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.css:241: outline: none; Do you need to specify none here? I would guess none is the default value.
https://codereview.chromium.org/2670833005/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui.css (right): https://codereview.chromium.org/2670833005/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.css:212: align-items: center; On 2017/02/03 16:37:05, bshe wrote: > I am wondering what's the element that you want to center? It doesn't look like > suggestion has any child element? This provides vertical alignment of the text within the div. It works for display:flex elements. https://codereview.chromium.org/2670833005/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.css:215: border-right: 5px solid transparent; On 2017/02/03 16:37:05, bshe wrote: > I am not sure why do you need transparent border here. Can you add comment about > it? Here's the reason (it's a bit weird): - We need margin on the suggestions - The text overflow must be hidden, so we can't use padding (text overflows into padding). - The background color needs to extend through this margin, so that when the suggestion background color changes, the margin changes colour too (due to being transparent). In terms of commenting, I'm not sure there's a point. Technically one could comment the need for every css property, but it's easy to turn off the properties and see what breaks. :-) Thoughts? https://codereview.chromium.org/2670833005/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.css:241: outline: none; On 2017/02/03 16:37:05, bshe wrote: > Do you need to specify none here? I would guess none is the default value. This is needed for input fields - otherwise, when the field is selected, Chrome draws an orange border on the field to "help us". I'll double check that the border property is also needed.
https://codereview.chromium.org/2670833005/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui.css (right): https://codereview.chromium.org/2670833005/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.css:241: outline: none; On 2017/02/03 20:24:24, cjgrant wrote: > On 2017/02/03 16:37:05, bshe wrote: > > Do you need to specify none here? I would guess none is the default value. > > This is needed for input fields - otherwise, when the field is selected, Chrome > draws an orange border on the field to "help us". I'll double check that the > border property is also needed. Re-verified. Both border:none and outline:none are needed to prevent the browser from putting a box around the input field at certain times.
On 2017/02/03 20:28:15, cjgrant wrote: > https://codereview.chromium.org/2670833005/diff/1/chrome/browser/resources/vr... > File chrome/browser/resources/vr_shell/vr_shell_ui.css (right): > > https://codereview.chromium.org/2670833005/diff/1/chrome/browser/resources/vr... > chrome/browser/resources/vr_shell/vr_shell_ui.css:241: outline: none; > On 2017/02/03 20:24:24, cjgrant wrote: > > On 2017/02/03 16:37:05, bshe wrote: > > > Do you need to specify none here? I would guess none is the default value. > > > > This is needed for input fields - otherwise, when the field is selected, > Chrome > > draws an orange border on the field to "help us". I'll double check that the > > border property is also needed. > > Re-verified. Both border:none and outline:none are needed to prevent the > browser from putting a box around the input field at certain times. lgtm
https://codereview.chromium.org/2670833005/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui.css (right): https://codereview.chromium.org/2670833005/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.css:212: align-items: center; On 2017/02/03 20:24:25, cjgrant wrote: > On 2017/02/03 16:37:05, bshe wrote: > > I am wondering what's the element that you want to center? It doesn't look > like > > suggestion has any child element? > > This provides vertical alignment of the text within the div. It works for > display:flex elements. Acknowledged. https://codereview.chromium.org/2670833005/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.css:215: border-right: 5px solid transparent; On 2017/02/03 20:24:25, cjgrant wrote: > On 2017/02/03 16:37:05, bshe wrote: > > I am not sure why do you need transparent border here. Can you add comment > about > > it? > > Here's the reason (it's a bit weird): > - We need margin on the suggestions > - The text overflow must be hidden, so we can't use padding (text overflows into > padding). > - The background color needs to extend through this margin, so that when the > suggestion background color changes, the margin changes colour too (due to being > transparent). > > In terms of commenting, I'm not sure there's a point. Technically one could > comment the need for every css property, but it's easy to turn off the > properties and see what breaks. :-) Thoughts? I see. So this is not very obvious and that's why I suggest add comment so it is easier to understand when reading the code. Turn off the property to see what breaks is not as efficient as reading comment. That being said, it is my personal style to add comments when my code is not very obvious. So I wont impose it on you. Feel free to submit as it is. https://codereview.chromium.org/2670833005/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.css:241: outline: none; On 2017/02/03 20:28:15, cjgrant wrote: > On 2017/02/03 20:24:24, cjgrant wrote: > > On 2017/02/03 16:37:05, bshe wrote: > > > Do you need to specify none here? I would guess none is the default value. > > > > This is needed for input fields - otherwise, when the field is selected, > Chrome > > draws an orange border on the field to "help us". I'll double check that the > > border property is also needed. > > Re-verified. Both border:none and outline:none are needed to prevent the > browser from putting a box around the input field at certain times. Interesing. ack
https://codereview.chromium.org/2670833005/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui.css (right): https://codereview.chromium.org/2670833005/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.css:215: border-right: 5px solid transparent; On 2017/02/03 21:15:37, bshe wrote: > On 2017/02/03 20:24:25, cjgrant wrote: > > On 2017/02/03 16:37:05, bshe wrote: > > > I am not sure why do you need transparent border here. Can you add comment > > about > > > it? > > > > Here's the reason (it's a bit weird): > > - We need margin on the suggestions > > - The text overflow must be hidden, so we can't use padding (text overflows > into > > padding). > > - The background color needs to extend through this margin, so that when the > > suggestion background color changes, the margin changes colour too (due to > being > > transparent). > > > > In terms of commenting, I'm not sure there's a point. Technically one could > > comment the need for every css property, but it's easy to turn off the > > properties and see what breaks. :-) Thoughts? > > I see. So this is not very obvious and that's why I suggest add comment so it is > easier to > understand when reading the code. Turn off the property to see what breaks is > not as efficient > as reading comment. > That being said, it is my personal style to add comments when my code is not > very obvious. So > I wont impose it on you. Feel free to submit as it is. I added a few comments to the odd-looking properties as you suggested. You are right that clear code is more important than pretty-looking code.
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/2670833005/#ps20001 (title: "Add CSS comments to unintuitive properties, as per Biao's suggestion.")
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 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/2670833005/#ps40001 (title: "Fix closure compiler failure. Yay closure compiler.")
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": 40001, "attempt_start_ts": 1486398616092890,
"parent_rev": "46d8850d22c89856e2ba2a0b294e9dcb3d995f15", "commit_rev":
"db713b182a14c3aacabed95d3b28f8885b1a633f"}
Message was sent while issue was closed.
Description was changed from ========== Improve VR omnibox styling and function - Improve the spacing and alignment of suggestions. - Clear suggestions when doing a navigation action. - Tweak the "Reload UI" debugging button to be more usable. BUG=641508 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Improve VR omnibox styling and function - Improve the spacing and alignment of suggestions. - Clear suggestions when doing a navigation action. - Tweak the "Reload UI" debugging button to be more usable. BUG=641508 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2670833005 Cr-Commit-Position: refs/heads/master@{#448291} Committed: https://chromium.googlesource.com/chromium/src/+/db713b182a14c3aacabed95d3b28... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/db713b182a14c3aacabed95d3b28... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
