Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2990963002:
config_service: focus search bar on page load (Closed)
|
Created:
3 years, 4 months ago by ayanaadylova Modified:
3 years, 4 months ago CC:
chromium-reviews, infra-reviews+luci-py_chromium.org Target Ref:
refs/heads/master Project:
luci-py Visibility:
Public. |
Descriptionconfig_service: focus search bar on page load
BUG=749995
Review-Url: https://codereview.chromium.org/2990963002
Committed: https://github.com/luci/luci-py/commit/229ed5303de83d33cf61dc0e692548fa344e29aa
Patch Set 1 #
Total comments: 3
Patch Set 2 : Change timeout to event listener. #Messages
Total messages: 12 (5 generated)
ayanaadylova@google.com changed reviewers: + hinoka@chromium.org, sergeyberezin@chromium.org
PTAL
LGTM + comment. Thanks! https://codereview.chromium.org/2990963002/diff/1/appengine/config_service/ui... File appengine/config_service/ui/src/config-ui/front-page.html (right): https://codereview.chromium.org/2990963002/diff/1/appengine/config_service/ui... appengine/config_service/ui/src/config-ui/front-page.html:154: this.$['searchBar'].focus(); Any reason why focus is inside a timeout rather than called directly?
https://codereview.chromium.org/2990963002/diff/1/appengine/config_service/ui... File appengine/config_service/ui/src/config-ui/front-page.html (right): https://codereview.chromium.org/2990963002/diff/1/appengine/config_service/ui... appengine/config_service/ui/src/config-ui/front-page.html:154: this.$['searchBar'].focus(); On 2017/07/31 18:37:15, Sergey Berezin wrote: > Any reason why focus is inside a timeout rather than called directly? Calling focus directly does not actually focus on the search bar. I think it is because <paper-search-bar> is not ready when we try to focus on it (despite being inside this custom element's ready callback). Instead of setting timeout I can also add event listener to the WebComponentsReady that would be called when all components are initialized.
https://codereview.chromium.org/2990963002/diff/1/appengine/config_service/ui... File appengine/config_service/ui/src/config-ui/front-page.html (right): https://codereview.chromium.org/2990963002/diff/1/appengine/config_service/ui... appengine/config_service/ui/src/config-ui/front-page.html:154: this.$['searchBar'].focus(); On 2017/07/31 at 18:51:52, ayanaadylova wrote: > On 2017/07/31 18:37:15, Sergey Berezin wrote: > > Any reason why focus is inside a timeout rather than called directly? > > Calling focus directly does not actually focus on the search bar. I think it is because <paper-search-bar> is not ready when we try to focus on it (despite being inside this custom element's ready callback). Instead of setting timeout I can also add event listener to the WebComponentsReady that would be called when all components are initialized. Indeed, I'd prefer an event trigger rather than a timeout, it's more reliable.
On 2017/07/31 20:31:45, Sergey Berezin wrote: > https://codereview.chromium.org/2990963002/diff/1/appengine/config_service/ui... > File appengine/config_service/ui/src/config-ui/front-page.html (right): > > https://codereview.chromium.org/2990963002/diff/1/appengine/config_service/ui... > appengine/config_service/ui/src/config-ui/front-page.html:154: > this.$['searchBar'].focus(); > On 2017/07/31 at 18:51:52, ayanaadylova wrote: > > On 2017/07/31 18:37:15, Sergey Berezin wrote: > > > Any reason why focus is inside a timeout rather than called directly? > > > > Calling focus directly does not actually focus on the search bar. I think it > is because <paper-search-bar> is not ready when we try to focus on it (despite > being inside this custom element's ready callback). Instead of setting timeout I > can also add event listener to the WebComponentsReady that would be called when > all components are initialized. > > Indeed, I'd prefer an event trigger rather than a timeout, it's more reliable. Ok, thanks!
The CQ bit was checked by ayanaadylova@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyberezin@chromium.org Link to the patchset: https://codereview.chromium.org/2990963002/#ps20001 (title: "Change timeout to event listener.")
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": 20001, "attempt_start_ts": 1501533548628540,
"parent_rev": "f29017a1537bceac4447ec928fb54915251329c1", "commit_rev":
"229ed5303de83d33cf61dc0e692548fa344e29aa"}
Message was sent while issue was closed.
Description was changed from ========== config_service: focus search bar on page load BUG=749995 ========== to ========== config_service: focus search bar on page load BUG=749995 Review-Url: https://codereview.chromium.org/2990963002 Committed: https://github.com/luci/luci-py/commit/229ed5303de83d33cf61dc0e692548fa344e29aa ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://github.com/luci/luci-py/commit/229ed5303de83d33cf61dc0e692548fa344e29aa |
