Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2935763003:
config_service: improve landing page (Closed)
Descriptionconfig_service: improve landing page
config sets are now downloaded and searchable, but not clickable
BUG=730832
Review-Url: https://codereview.chromium.org/2935763003
Committed: https://github.com/luci/luci-py/commit/2279363402929b7d5e45d5bb6591addab7824f2c
Patch Set 1 #
Total comments: 9
Patch Set 2 : Resized chromium image, differentiated between empty and loading config set, fixed _getConfigs to u… #
Total comments: 7
Patch Set 3 : Fixed empty vs. loading to reflect current search instead of all configs #
Total comments: 7
Patch Set 4 : Switched conditionals, created static reference to images #
Total comments: 6
Patch Set 5 : Final small tweaks on front page element. #Patch Set 6 : Fixed app.yaml bug #
Total comments: 43
Patch Set 7 : Fixed nodir's comments #
Total comments: 5
Patch Set 8 : Fixed app.yaml issue, set _isEmpty to be a function in front-page.html #
Total comments: 2
Patch Set 9 : Small syntax fixes. #Patch Set 10 : Changed configList to configSetList for clarity. #
Total comments: 5
Patch Set 11 : Fixed final nits. #Messages
Total messages: 41 (14 generated)
cwpayton@google.com changed reviewers: + hinoka@google.com, sergeyberezin@google.com
PTAL
Description was changed from ========== Improved on landing page; config sets are now downloaded and searchable, but not indexable BUG=730832 ========== to ========== Improved on landing page; config sets are now downloaded and searchable, but not clickable BUG=730832 ==========
Great start! I have a bunch of initial comments. apps.py: is this file here by accident? (it's empty). Let's also add tests for the new elements, and delete config-ui_test.html. https://codereview.chromium.org/2935763003/diff/1/appengine/config_service/ui... File appengine/config_service/ui/src/config-ui/front-page.html (right): https://codereview.chromium.org/2935763003/diff/1/appengine/config_service/ui... appengine/config_service/ui/src/config-ui/front-page.html:52: <image class="logo" src="../../images/chromium.png"/> nit: how big is the icon file? It may be useful to scale it down to max. expected resolution to save network bandwidth. https://codereview.chromium.org/2935763003/diff/1/appengine/config_service/ui... appengine/config_service/ui/src/config-ui/front-page.html:76: <config-card name="Fetching configs sets..."></config-card> For app instances that do not (yet) have config sets, this message will be confusing. Perhaps, you want to distinguish not-yet-loaded state from loaded-but-empty, so you can give a different message (like "no configs found"). In fact, renaming "empty" to "isLoading" (which is what it really is) and checking searchList.length gives you all the combinations. https://codereview.chromium.org/2935763003/diff/1/appengine/config_service/ui... appengine/config_service/ui/src/config-ui/front-page.html:91: empty: { s/empty/isLoading/ - see the comment above. https://codereview.chromium.org/2935763003/diff/1/appengine/config_service/ui... appengine/config_service/ui/src/config-ui/front-page.html:108: value: "https://luci-config.appspot.com/_ah/api/config/v1/config-sets" So this UI will only work on luci-config instance. Would it still work if we omit the server part of the URL? E.g. just "/_ah/api/..." ? This should then work on any instance. https://codereview.chromium.org/2935763003/diff/1/appengine/config_service/ui... appengine/config_service/ui/src/config-ui/front-page.html:114: this.searchList = this.configList; You may want to call _searched() here instead. Consider this: a user opens the UI page on a slow network, and while configs are still loading, types in a search query. Currently, once the configs load, they'll still display all the results, and will only update once the user modifies the search query. It would be better to start displaying already filtered results.
PTAL https://codereview.chromium.org/2935763003/diff/1/appengine/config_service/ui... File appengine/config_service/ui/src/config-ui/front-page.html (right): https://codereview.chromium.org/2935763003/diff/1/appengine/config_service/ui... appengine/config_service/ui/src/config-ui/front-page.html:52: <image class="logo" src="../../images/chromium.png"/> On 2017/06/12 20:01:41, Sergey Berezin (google) wrote: > nit: how big is the icon file? It may be useful to scale it down to max. > expected resolution to save network bandwidth. Done. https://codereview.chromium.org/2935763003/diff/1/appengine/config_service/ui... appengine/config_service/ui/src/config-ui/front-page.html:76: <config-card name="Fetching configs sets..."></config-card> On 2017/06/12 20:01:41, Sergey Berezin (google) wrote: > For app instances that do not (yet) have config sets, this message will be > confusing. Perhaps, you want to distinguish not-yet-loaded state from > loaded-but-empty, so you can give a different message (like "no configs found"). > > In fact, renaming "empty" to "isLoading" (which is what it really is) and > checking searchList.length gives you all the combinations. Done. https://codereview.chromium.org/2935763003/diff/1/appengine/config_service/ui... appengine/config_service/ui/src/config-ui/front-page.html:91: empty: { On 2017/06/12 20:01:41, Sergey Berezin (google) wrote: > s/empty/isLoading/ - see the comment above. Done. https://codereview.chromium.org/2935763003/diff/1/appengine/config_service/ui... appengine/config_service/ui/src/config-ui/front-page.html:114: this.searchList = this.configList; On 2017/06/12 20:01:41, Sergey Berezin (google) wrote: > You may want to call _searched() here instead. > Consider this: a user opens the UI page on a slow network, and while configs are > still loading, types in a search query. > Currently, once the configs load, they'll still display all the results, and > will only update once the user modifies the search query. > It would be better to start displaying already filtered results. Done.
Almost there - a few more polishing comments. - Please remove apps.py - Please post a screenshot of the rendered page More comments below. Thanks! https://codereview.chromium.org/2935763003/diff/40001/appengine/config_servic... File appengine/config_service/ui/src/config-ui/front-page.html (right): https://codereview.chromium.org/2935763003/diff/40001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:122: value: "https://luci-config.appspot.com/_ah/api/config/v1/config-sets" I'd say, let's remove the domain part of the URL from the code before landing this. It's easy to put it back locally for testing, and eventually we should come up with a way to serve test data locally. https://codereview.chromium.org/2935763003/diff/40001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:128: this.searchList = this._searched(); No need to assign to this.searchList; _searched() already updates it. https://codereview.chromium.org/2935763003/diff/40001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:145: return this.searchList; No need to return the result, since it modifies this.seachList. https://codereview.chromium.org/2935763003/diff/40001/appengine/config_servic... File appengine/config_service/ui/test/config-ui/front-page_test.html (right): https://codereview.chromium.org/2935763003/diff/40001/appengine/config_servic... appengine/config_service/ui/test/config-ui/front-page_test.html:30: // TODO(cwpayton): write tests for the front page element. Let's actually write some tests, since you now have non-trivial functionality.
https://codereview.chromium.org/2935763003/diff/20001/appengine/config_servic... File appengine/config_service/ui/src/config-ui/front-page.html (right): https://codereview.chromium.org/2935763003/diff/20001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:52: <image class="logo" src="../../images/chromium.png"/> Use absolute URLs whenever possible. Should be /images/chromium.png https://codereview.chromium.org/2935763003/diff/20001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:53: <div class="title" main-title>Configuration Service (not fully implemented)</div> nit: 80 char https://codereview.chromium.org/2935763003/diff/20001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:70: <template is="dom-if" if="{{_not(isLoading)}}"> does if="{{!isLoading}}" not work? Also in general, it's preferred to have positive conditions first, then negative (unless it makes more sense the other way) https://codereview.chromium.org/2935763003/diff/20001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:118: // TODO(cwpayton): Fix URL to reflect specific instances of the luci nit: 80 char
PTAL https://codereview.chromium.org/2935763003/diff/20001/appengine/config_servic... File appengine/config_service/ui/src/config-ui/front-page.html (right): https://codereview.chromium.org/2935763003/diff/20001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:53: <div class="title" main-title>Configuration Service (not fully implemented)</div> On 2017/06/12 22:26:20, Ryan Tseng wrote: > nit: 80 char Done. https://codereview.chromium.org/2935763003/diff/20001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:70: <template is="dom-if" if="{{_not(isLoading)}}"> On 2017/06/12 22:26:20, Ryan Tseng wrote: > does if="{{!isLoading}}" not work? > > Also in general, it's preferred to have positive conditions first, then negative > (unless it makes more sense the other way) if="{{!isLoading}}" does not work unfortunately. I did flip the conditionals to positive first. https://codereview.chromium.org/2935763003/diff/20001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:118: // TODO(cwpayton): Fix URL to reflect specific instances of the luci On 2017/06/12 22:26:20, Ryan Tseng wrote: > nit: 80 char Done. https://codereview.chromium.org/2935763003/diff/40001/appengine/config_servic... File appengine/config_service/ui/src/config-ui/front-page.html (right): https://codereview.chromium.org/2935763003/diff/40001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:122: value: "https://luci-config.appspot.com/_ah/api/config/v1/config-sets" On 2017/06/12 22:19:49, Sergey Berezin (google) wrote: > I'd say, let's remove the domain part of the URL from the code before landing > this. It's easy to put it back locally for testing, and eventually we should > come up with a way to serve test data locally. Done. https://codereview.chromium.org/2935763003/diff/40001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:128: this.searchList = this._searched(); On 2017/06/12 22:19:49, Sergey Berezin (google) wrote: > No need to assign to this.searchList; _searched() already updates it. This needs to stay in because searchList has no initial value. If this line is removed, then when a user first lands on a page, they will get the "no config sets found" message until they actually start typing a search query. https://codereview.chromium.org/2935763003/diff/40001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:145: return this.searchList; On 2017/06/12 22:19:49, Sergey Berezin (google) wrote: > No need to return the result, since it modifies this.seachList. The return statement is in so that searchList will be returned when _searched is called in line 128
lgtm % comments https://codereview.chromium.org/2935763003/diff/60001/appengine/config_servic... File appengine/config_service/app.yaml (right): https://codereview.chromium.org/2935763003/diff/60001/appengine/config_servic... appengine/config_service/app.yaml:17: - url: /images nit: /static/images or /static/img Just by convention we put everything static under a /static/ folder https://codereview.chromium.org/2935763003/diff/60001/appengine/config_servic... File appengine/config_service/apps.py (left): https://codereview.chromium.org/2935763003/diff/60001/appengine/config_servic... appengine/config_service/apps.py:1: # Copyright 2015 The LUCI Authors. All rights reserved. Oh this is an actual file. No you don't want to delete it then, just run "git checkout origin/master -- ./apps.py" https://codereview.chromium.org/2935763003/diff/60001/appengine/config_servic... File appengine/config_service/ui/src/config-ui/front-page.html (right): https://codereview.chromium.org/2935763003/diff/60001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:53: <div class="title" You can do the line break at the tag level, so <div class="..." main title> Configuration.... </div> https://codereview.chromium.org/2935763003/diff/60001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:59: auto nit: 2 indents, since it's still in a tag
The CQ bit was checked by cwpayton@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from hinoka@google.com Link to the patchset: https://codereview.chromium.org/2935763003/#ps80001 (title: "Final small tweaks on front page element.")
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: Luci-py Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/36b7966fbe11a010)
The CQ bit was checked by cwpayton@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from hinoka@google.com Link to the patchset: https://codereview.chromium.org/2935763003/#ps90001 (title: "Fixed app.yaml bug")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
hinoka@google.com changed reviewers: + nodir@chromium.org
+nodir, who is an owner
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Luci-py Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/36b7a1bda70ce410)
Description was changed from ========== Improved on landing page; config sets are now downloaded and searchable, but not clickable BUG=730832 ========== to ========== config_service: improve landing page config sets are now downloaded and searchable, but not clickable BUG=730832 ==========
I've updated CL description to start with "config_service: " so that 1) it matches other commits in ///appengine/config_service, i.e. "config_service: " prefix. Use imperative style. 2) first line must be a subject. then blank line, then details. still reviewing
is there a demo? https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... File appengine/config_service/ui/.gitignore (right): https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/.gitignore:2: *~ what does this ignore? https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... File appengine/config_service/ui/src/config-ui/config-card.html (right): https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/config-card.html:4: that can be found in the LICENSE file. use same indentation for all lines https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/config-card.html:8: <link rel="import" href="../../bower_components/paper-item/paper-item.html"> sort imports here and in other files https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/config-card.html:25: {{name}} use [[]] for read-only binding https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... File appengine/config_service/ui/src/config-ui/front-page.html (right): https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:60: url="{{url}}" why this is a property binding as opposed to a constant? the property is never changed. I think it would be simpler to inline the URL here https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:71: <config-card name="Fetching configs sets..."></config-card> s/configs sets/config sets/ https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:71: <config-card name="Fetching configs sets..."></config-card> why is it a config card? It does not look right. If something extra is added to the config-card, it will appear here, even if it shouldn't. If there is a specific style and you want "Fetching config sets..." to have the same style, I recommend to copy the style here https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:73: <template is="dom-if" if="{{_not(isLoading)}}"> use [[]] for read-only data bindings https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:74: <template is="dom-if" if="{{isEmpty}}"> use [[]] for read-only data bindings https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:77: <template is="dom-if" if="{{_not(isEmpty)}}"> use [[]] for read-only data bindings https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:98: value: true FWIU isEmpty is true if and only if len(searchList) == 0 please implement it as a function, like you do with _not, and call it in the template above. Then you don't have to set in searched() https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:106: search: { responsibility of this property is not obvious from this name. It could be query, or results. Standard name for this is "query" (see also "query" attribute of paper-search-bar). Please rename to "query". https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:113: value: [] see https://www.polymer-project.org/1.0/docs/devguide/properties#configure-values this should use a function that returns [] https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:119: } i don't think this property is needed https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:122: _getConfigs: function(event) { name "_getConfigs" looks like it will get configs for a caller, but in fact it is not. It should be called when configs are got. Please give it a name that more clearly expressed that, for example "_onGotConfigs" https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:124: this.searchList = this._searched(); _searched() sets this.searchList and returns it. This line sets it again to the same value. This isn't right. I think searched() should either do no side effects (not change this.searchList) and return a value, or it should do side effects, not return it and this line should not set this.searchList. also, name "searchList" may be interpreted as "things that we are search in", as opposed to "search results". Please rename this field to searchResults https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:133: _searched: function() { responsibility of this function is not obvious from name "searched". One may interpret it as "should be called after search is performed", but it is not. This function essentially updates search results. Please rename to updateSearchResults() or alike https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:135: this.configList.forEach((elem) => { parens around elem are unnecessary https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:139: }); this essentially filters this.configList. Using filter method would be simpler https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje... https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:141: return this.searchList; there is no need to return this.searchList. If someone needs it, they can read the list from this.searchList after calling this function
Screenshots of the landing page: Loading: https://screenshot.googleplex.com/2SvxGLhejHF No search query: https://screenshot.googleplex.com/mTsjDWETgwu Search query: https://screenshot.googleplex.com/emJPxZABwas Invalid search: https://screenshot.googleplex.com/jdyg17ENtLM
On 2017/06/13 16:02:31, cwpayton wrote: > Screenshots of the landing page: > > Loading: https://screenshot.googleplex.com/2SvxGLhejHF > > No search query: https://screenshot.googleplex.com/mTsjDWETgwu > > Search query: https://screenshot.googleplex.com/emJPxZABwas > > Invalid search: https://screenshot.googleplex.com/jdyg17ENtLM by demo I meant a deployed version with these changes. I've just fetched your patch: $ git new-branch cwpayton $ git cl patch https://codereview.chromium.org/2935763003/ and tried to upload $ ./tools/gae upload -A luci-config-dev this failed with an error that upload and static_dir are mutually exclusive. After fixing that, it uploaded 2896-ff1ee35-tainted-nodir-dot-luci-config-dev.appspot.com looks like currently handlers are not hooked yet. Would be nice to see the app in action, but not urgent. Screenshots are fine for now.
https://codereview.chromium.org/2935763003/diff/60001/appengine/config_servic... File appengine/config_service/ui/src/config-ui/front-page.html (right): https://codereview.chromium.org/2935763003/diff/60001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:53: <div class="title" On 2017/06/12 23:47:17, Ryan Tseng wrote: > You can do the line break at the tag level, so > > <div class="..." main title> > Configuration.... > </div> Done. https://codereview.chromium.org/2935763003/diff/60001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:59: auto On 2017/06/12 23:47:17, Ryan Tseng wrote: > nit: 2 indents, since it's still in a tag Done. https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... File appengine/config_service/ui/.gitignore (right): https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/.gitignore:2: *~ On 2017/06/13 02:10:14, nodir wrote: > what does this ignore? On occasion I'll edit quick things in emacs rather than sublime. Unfortunately emacs has an annoying backup feature that creates duplicate files with ~ at the end. So if I edit bower.json with emacs, I may end up with a bower.json~ file. So I'm telling git to ignore these files in case I forget to delete them. https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... File appengine/config_service/ui/src/config-ui/front-page.html (right): https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:60: url="{{url}}" On 2017/06/13 02:10:15, nodir wrote: > why this is a property binding as opposed to a constant? > the property is never changed. I think it would be simpler to inline the URL > here Done. https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:71: <config-card name="Fetching configs sets..."></config-card> On 2017/06/13 02:10:15, nodir wrote: > s/configs sets/config sets/ Done. https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:71: <config-card name="Fetching configs sets..."></config-card> On 2017/06/13 02:10:15, nodir wrote: > why is it a config card? It does not look right. If something extra is added to > the config-card, it will appear here, even if it shouldn't. If there is a > specific style and you want "Fetching config sets..." to have the same style, I > recommend to copy the style here Done. https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:73: <template is="dom-if" if="{{_not(isLoading)}}"> On 2017/06/13 02:10:14, nodir wrote: > use [[]] for read-only data bindings Done. https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:74: <template is="dom-if" if="{{isEmpty}}"> On 2017/06/13 02:10:14, nodir wrote: > use [[]] for read-only data bindings Done. https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:77: <template is="dom-if" if="{{_not(isEmpty)}}"> On 2017/06/13 02:10:14, nodir wrote: > use [[]] for read-only data bindings Done. https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:98: value: true On 2017/06/13 02:10:15, nodir wrote: > FWIU isEmpty is true if and only if len(searchList) == 0 > please implement it as a function, like you do with _not, and call it in the > template above. Then you don't have to set in searched() If I implement this as a function, it only gets called once by the elements that depend on it when the page initially loads. If it's a property, then the display will adjust accordingly as the user types their search. This way if a user searches for "configdoesnotexist" the search results will be empty and they will get "no config sets found." https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:106: search: { On 2017/06/13 02:10:15, nodir wrote: > responsibility of this property is not obvious from this name. It could be > query, or results. Standard name for this is "query" (see also "query" attribute > of paper-search-bar). Please rename to "query". Done. https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:113: value: [] On 2017/06/13 02:10:14, nodir wrote: > see > https://www.polymer-project.org/1.0/docs/devguide/properties#configure-values > this should use a function that returns [] Done. Does it matter whether I use arrow function notation for this? https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:119: } On 2017/06/13 02:10:15, nodir wrote: > i don't think this property is needed Done. https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:122: _getConfigs: function(event) { On 2017/06/13 02:10:15, nodir wrote: > name "_getConfigs" looks like it will get configs for a caller, but in fact it > is not. It should be called when configs are got. Please give it a name that > more clearly expressed that, for example "_onGotConfigs" Done. https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:124: this.searchList = this._searched(); On 2017/06/13 02:10:14, nodir wrote: > _searched() sets this.searchList and returns it. This line sets it again to the > same value. This isn't right. > > I think searched() should either do no side effects (not change this.searchList) > and return a value, or it should do side effects, not return it and this line > should not set this.searchList. > > also, name "searchList" may be interpreted as "things that we are search in", as > opposed to "search results". Please rename this field to searchResults Done. https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:133: _searched: function() { On 2017/06/13 02:10:14, nodir wrote: > responsibility of this function is not obvious from name "searched". One may > interpret it as "should be called after search is performed", but it is not. > > This function essentially updates search results. Please rename to > updateSearchResults() or alike Done. https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:135: this.configList.forEach((elem) => { On 2017/06/13 02:10:15, nodir wrote: > parens around elem are unnecessary Done. https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:139: }); On 2017/06/13 02:10:14, nodir wrote: > this essentially filters this.configList. Using filter method would be simpler > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje... Done. https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:141: return this.searchList; On 2017/06/13 02:10:14, nodir wrote: > there is no need to return this.searchList. If someone needs it, they can read > the list from this.searchList after calling this function Done.
https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... File appengine/config_service/app.yaml (right): https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/app.yaml:19: upload: ui/static/images upload and static_dir are mutually exclusive
PTAL https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... File appengine/config_service/app.yaml (right): https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/app.yaml:19: upload: ui/static/images On 2017/06/13 16:35:03, nodir wrote: > upload and static_dir are mutually exclusive Done.
some of the comments are for previous CL https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... File appengine/config_service/ui/src/config-ui/front-page.html (right): https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:98: value: true On 2017/06/13 16:22:14, cwpayton wrote: > On 2017/06/13 02:10:15, nodir wrote: > > FWIU isEmpty is true if and only if len(searchList) == 0 > > please implement it as a function, like you do with _not, and call it in the > > template above. Then you don't have to set in searched() > > If I implement this as a function, it only gets called once by the elements that > depend on it when the page initially loads. If it's a property, then the display > will adjust accordingly as the user types their search. This way if a user > searches for "configdoesnotexist" the search results will be empty and they will > get "no config sets found." i meant that isEmpty should accept search results as a param, so that polymer knows that result of isEmpty depends on it and calls it every time searchResults change https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:113: value: [] On 2017/06/13 16:22:15, cwpayton wrote: > On 2017/06/13 02:10:14, nodir wrote: > > see > > https://www.polymer-project.org/1.0/docs/devguide/properties#configure-values > > this should use a function that returns [] > > Done. Does it matter whether I use arrow function notation for this? arrow function is nice https://codereview.chromium.org/2935763003/diff/110001/appengine/config_servi... File appengine/config_service/app.yaml (right): https://codereview.chromium.org/2935763003/diff/110001/appengine/config_servi... appengine/config_service/app.yaml:19: upload: ui/static/images upload and static_dir are mutually exclusive https://codereview.chromium.org/2935763003/diff/110001/appengine/config_servi... File appengine/config_service/ui/src/config-ui/front-page.html (right): https://codereview.chromium.org/2935763003/diff/110001/appengine/config_servi... appengine/config_service/ui/src/config-ui/front-page.html:66: url="https://luci-config.appspot.com/_ah/api/config/v1/config-sets" this is probably slow. currently this loads information that you don't need and loading that information is expensive. i can add parameters to limit the response only to names of config sets and it will work faster no action needed in this CL though https://codereview.chromium.org/2935763003/diff/110001/appengine/config_servi... appengine/config_service/ui/src/config-ui/front-page.html:68: on-response="_onGotConfigs"> apologies, I realized that a better name would be "_onGotConfigSets" since it is the config sets we are getting here https://codereview.chromium.org/2935763003/diff/110001/appengine/config_servi... appengine/config_service/ui/src/config-ui/front-page.html:126: this.isEmpty = this.searchResults.length === 0; _updateSearchResults already updates isEmpty, so this line is unnecessary https://codereview.chromium.org/2935763003/diff/110001/appengine/config_servi... appengine/config_service/ui/src/config-ui/front-page.html:138: }); arrow functions are best for this kind of inline functions for 2 reasons: 1) return is implied if you don't put {} 2) it captures "this" correctly, so L135 is unnecessary this.searchResults = this.configList.filter(e => e.config_set.includes(this.query)) https://codereview.chromium.org/2935763003/diff/130001/appengine/config_servi... File appengine/config_service/ui/src/config-ui/front-page.html (right): https://codereview.chromium.org/2935763003/diff/130001/appengine/config_servi... appengine/config_service/ui/src/config-ui/front-page.html:119: return searchResults.length === 0; this function does not actually care if the parameter is indeed search results or any other list, so it can be generalized to any list. Let's rename searchResults to "array"
https://codereview.chromium.org/2935763003/diff/130001/appengine/config_servi... File appengine/config_service/ui/src/config-ui/front-page.html (right): https://codereview.chromium.org/2935763003/diff/130001/appengine/config_servi... appengine/config_service/ui/src/config-ui/front-page.html:123: this.configList = event.detail.response.config_sets; I think this is configSetList, not configList. Config is a single config file.
PTAL https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... File appengine/config_service/ui/src/config-ui/front-page.html (right): https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:98: value: true On 2017/06/13 16:48:22, nodir wrote: > On 2017/06/13 16:22:14, cwpayton wrote: > > On 2017/06/13 02:10:15, nodir wrote: > > > FWIU isEmpty is true if and only if len(searchList) == 0 > > > please implement it as a function, like you do with _not, and call it in the > > > template above. Then you don't have to set in searched() > > > > If I implement this as a function, it only gets called once by the elements > that > > depend on it when the page initially loads. If it's a property, then the > display > > will adjust accordingly as the user types their search. This way if a user > > searches for "configdoesnotexist" the search results will be empty and they > will > > get "no config sets found." > > i meant that isEmpty should accept search results as a param, so that polymer > knows that result of isEmpty depends on it and calls it every time searchResults > change Done. https://codereview.chromium.org/2935763003/diff/90001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:113: value: [] On 2017/06/13 16:48:22, nodir wrote: > On 2017/06/13 16:22:15, cwpayton wrote: > > On 2017/06/13 02:10:14, nodir wrote: > > > see > > > > https://www.polymer-project.org/1.0/docs/devguide/properties#configure-values > > > this should use a function that returns [] > > > > Done. Does it matter whether I use arrow function notation for this? > > arrow function is nice Done.
lgtm % comments https://codereview.chromium.org/2935763003/diff/170001/appengine/config_servi... File appengine/config_service/ui/src/config-ui/front-page.html (right): https://codereview.chromium.org/2935763003/diff/170001/appengine/config_servi... appengine/config_service/ui/src/config-ui/front-page.html:99: value: [] use a function that returns an array here too https://codereview.chromium.org/2935763003/diff/170001/appengine/config_servi... appengine/config_service/ui/src/config-ui/front-page.html:129: return (!(bool)); nit: this parens unnecessary. This can be `return !bool;` https://codereview.chromium.org/2935763003/diff/170001/appengine/config_servi... appengine/config_service/ui/src/config-ui/front-page.html:133: var query = this.query; this variable is no longer necessary `this.query` should work inside the arrow function because it is same as outside. This applies to all arrow functions
https://codereview.chromium.org/2935763003/diff/170001/appengine/config_servi... File appengine/config_service/ui/src/config-ui/front-page.html (right): https://codereview.chromium.org/2935763003/diff/170001/appengine/config_servi... appengine/config_service/ui/src/config-ui/front-page.html:129: return (!(bool)); On 2017/06/13 17:13:56, nodir wrote: > nit: this parens unnecessary. This can be `return !bool;` or even shorter _not: b => !b
LGTM + one small nit from me. Thanks! https://codereview.chromium.org/2935763003/diff/170001/appengine/config_servi... File appengine/config_service/ui/src/config-ui/front-page.html (right): https://codereview.chromium.org/2935763003/diff/170001/appengine/config_servi... appengine/config_service/ui/src/config-ui/front-page.html:81: <config-card name="No config sets found."></config-card> nit: I'd leave it as a plain <div>, not a config-card. Otherwise it looks like a config set named "No config sets found", which is visually confusing.
The CQ bit was checked by cwpayton@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from hinoka@google.com, nodir@chromium.org, sergeyberezin@google.com Link to the patchset: https://codereview.chromium.org/2935763003/#ps190001 (title: "Fixed final nits.")
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": 190001, "attempt_start_ts": 1497375917611330,
"parent_rev": "d96c2be24ea3c769cce1cbe671af6786bb7ec1de", "commit_rev":
"2279363402929b7d5e45d5bb6591addab7824f2c"}
Message was sent while issue was closed.
Description was changed from ========== config_service: improve landing page config sets are now downloaded and searchable, but not clickable BUG=730832 ========== to ========== config_service: improve landing page config sets are now downloaded and searchable, but not clickable BUG=730832 Review-Url: https://codereview.chromium.org/2935763003 Committed: https://github.com/luci/luci-py/commit/2279363402929b7d5e45d5bb6591addab7824f2c ==========
Message was sent while issue was closed.
Committed patchset #11 (id:190001) as https://github.com/luci/luci-py/commit/2279363402929b7d5e45d5bb6591addab7824f2c |
