Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2570743002:
Keyboard settings: icon buttons for tappable rows (Closed)
|
Created:
4 years ago by michaelpg Modified:
4 years ago Reviewers:
hcarmona CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, arv+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionKeyboard settings: icon buttons for tappable rows
Add arrow button for Language settings row (which navigates to the
languages section).
Add external/popup button for Keyboard Shortcuts row (which opens the
external CrOS keyboard shortcuts overlay).
BUG=673925
R=hcarmona@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/77818ddc739721826a3dc6ab741b0b2f72f023df
Cr-Commit-Position: refs/heads/master@{#439918}
Patch Set 1 #
Total comments: 4
Messages
Total messages: 14 (6 generated)
Description was changed from ========== Keyboard settings: icon buttons for tappable rows Add arrow button for Language settings row (which navigates to the languages section). Add external/popup button for Keyboard Shortcuts row (which opens the external CrOS keyboard shortcuts overlay). BUG=none R=hcarmona@chromium.org ========== to ========== Keyboard settings: icon buttons for tappable rows Add arrow button for Language settings row (which navigates to the languages section). Add external/popup button for Keyboard Shortcuts row (which opens the external CrOS keyboard shortcuts overlay). BUG=none R=hcarmona@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
PTAL at this short CL to add buttons.
https://codereview.chromium.org/2570743002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/device_page/keyboard.html (right): https://codereview.chromium.org/2570743002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/keyboard.html:118: <button class="subpage-arrow" is="paper-icon-button-light"></button> subpage-arrow seems a bit odd to me because we're not navigating to a subpage. It also seems a bit odd because after clicking it, when we click back twice we are back at "Languages and Input" instead of "Device". But, that seems like a different issue. Do we have any other UI where we do this?
I'll leave this open pending the bug resolution. https://codereview.chromium.org/2570743002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/device_page/keyboard.html (right): https://codereview.chromium.org/2570743002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/keyboard.html:118: <button class="subpage-arrow" is="paper-icon-button-light"></button> On 2016/12/13 18:00:47, hcarmona wrote: > subpage-arrow seems a bit odd to me because we're not navigating to a subpage. > It also seems a bit odd because after clicking it, when we click back twice we > are back at "Languages and Input" instead of "Device". But, that seems like a > different issue. > > Do we have any other UI where we do this? Storage management does this (CrOS). I also filed crbug.com/673925.
Description was changed from ========== Keyboard settings: icon buttons for tappable rows Add arrow button for Language settings row (which navigates to the languages section). Add external/popup button for Keyboard Shortcuts row (which opens the external CrOS keyboard shortcuts overlay). BUG=none R=hcarmona@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Keyboard settings: icon buttons for tappable rows Add arrow button for Language settings row (which navigates to the languages section). Add external/popup button for Keyboard Shortcuts row (which opens the external CrOS keyboard shortcuts overlay). BUG=673925 R=hcarmona@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
PTAL. https://codereview.chromium.org/2570743002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/device_page/keyboard.html (right): https://codereview.chromium.org/2570743002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/keyboard.html:118: <button class="subpage-arrow" is="paper-icon-button-light"></button> On 2016/12/13 18:00:47, hcarmona wrote: > subpage-arrow seems a bit odd to me because we're not navigating to a subpage. > It also seems a bit odd because after clicking it, when we click back twice we > are back at "Languages and Input" instead of "Device". But, that seems like a > different issue. > > Do we have any other UI where we do this? See dbeam's replies at crbug.com/673925 -- I think these are the right icons to use.
lgtm https://codereview.chromium.org/2570743002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/device_page/keyboard.html (right): https://codereview.chromium.org/2570743002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/keyboard.html:118: <button class="subpage-arrow" is="paper-icon-button-light"></button> On 2016/12/17 07:11:15, michaelpg wrote: > On 2016/12/13 18:00:47, hcarmona wrote: > > subpage-arrow seems a bit odd to me because we're not navigating to a subpage. > > It also seems a bit odd because after clicking it, when we click back twice we > > are back at "Languages and Input" instead of "Device". But, that seems like a > > different issue. > > > > Do we have any other UI where we do this? > > See dbeam's replies at crbug.com/673925 -- I think these are the right icons to > use. Sounds good.
The CQ bit was checked by michaelpg@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": 1, "attempt_start_ts": 1482265672922660, "parent_rev":
"5ea3399cbc26de6e8c7bbf1dc09c65a91073a5a1", "commit_rev":
"aa5a3a47c167d30bfc22927990939a58f52fb25c"}
Message was sent while issue was closed.
Description was changed from ========== Keyboard settings: icon buttons for tappable rows Add arrow button for Language settings row (which navigates to the languages section). Add external/popup button for Keyboard Shortcuts row (which opens the external CrOS keyboard shortcuts overlay). BUG=673925 R=hcarmona@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Keyboard settings: icon buttons for tappable rows Add arrow button for Language settings row (which navigates to the languages section). Add external/popup button for Keyboard Shortcuts row (which opens the external CrOS keyboard shortcuts overlay). BUG=673925 R=hcarmona@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2570743002 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Keyboard settings: icon buttons for tappable rows Add arrow button for Language settings row (which navigates to the languages section). Add external/popup button for Keyboard Shortcuts row (which opens the external CrOS keyboard shortcuts overlay). BUG=673925 R=hcarmona@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2570743002 ========== to ========== Keyboard settings: icon buttons for tappable rows Add arrow button for Language settings row (which navigates to the languages section). Add external/popup button for Keyboard Shortcuts row (which opens the external CrOS keyboard shortcuts overlay). BUG=673925 R=hcarmona@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/77818ddc739721826a3dc6ab741b0b2f72f023df Cr-Commit-Position: refs/heads/master@{#439918} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/77818ddc739721826a3dc6ab741b0b2f72f023df Cr-Commit-Position: refs/heads/master@{#439918} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
