config_service: improve landing page

appengine/config_service/ui/src/config-ui/front-page.html

+<!--
+  Copyright 2017 The LUCI Authors. All rights reserved.
+  Use of this source code is governed under the Apache License, Version 2.0
+  that can be found in the LICENSE file.
+-->
+
+<link rel="import" href="../../bower_components/polymer/polymer.html">
+<link rel="import" href="../../bower_components/app-layout/app-layout.html">
+<link rel="import" href="../../bower_components/paper-search/paper-search-bar.html">
+<link rel="import" href="../../bower_components/paper-button/paper-button.html">
+<link rel="import" href="../../bower_components/iron-ajax/iron-ajax.html">
+<link rel="import" href="config-card.html">
+
+<dom-module id="front-page">
+  <template>
+    <style>
+      app-toolbar {
+        background-color: #efefef;
+        color: #232323;
+      }
+
+      app-header {
+        @apply --layout-fixed-top;
+      }
+
+      .logo {
+        height: 100%;
+      }
+
+      .title {
+        padding-left: 1%;
+        font-size: 150%;
+        font-family: sans-serif;
+      }
+
+      .search-bar {
+        padding-top: 7%;
+        padding-bottom: 2%;
+      }
+
+      paper-search-bar {
+        border-style: solid;
+        border-width: 2px;
+        width: 40%;
+        height: 100%;
+        margin: auto;
+      }
+    </style>
+
+    <app-header reveals>
+      <app-toolbar>
+        <image class="logo" src="/static/images/chromium.png"/>
+        <div class="title" main-title>Configuration Service (not fully implemented)</div>
+      </app-toolbar>
+    </app-header>
+
+    <iron-ajax
+        auto
+        id="requestConfigs"
+        url="{{url}}"

[nodir, 2017/06/13 02:10:15]

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

[cwpayton, 2017/06/13 16:22:15]

Done.

+        handle-as="json"
+        on-response="_getConfigs">
+    </iron-ajax>
+
+    <div class="search-bar">
+      <paper-search-bar query="{{search}}"></paper-search-bar>
+    </div>
+    
+    <div class="config-list">
+      <template is="dom-if" if="{{isLoading}}">
+        <config-card name="Fetching configs sets..."></config-card>

[nodir, 2017/06/13 02:10:15]

s/configs sets/config sets/

[nodir, 2017/06/13 02:10:15]

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

[cwpayton, 2017/06/13 16:22:14]

Done.

[cwpayton, 2017/06/13 16:22:14]

Done.

+      </template>
+      <template is="dom-if" if="{{_not(isLoading)}}">

[nodir, 2017/06/13 02:10:14]

use [[]] for read-only data bindings

[cwpayton, 2017/06/13 16:22:15]

Done.

+        <template is="dom-if" if="{{isEmpty}}">

[nodir, 2017/06/13 02:10:14]

use [[]] for read-only data bindings

[cwpayton, 2017/06/13 16:22:14]

Done.

+          <config-card name="No config sets found."></config-card>
+        </template>
+        <template is="dom-if" if="{{_not(isEmpty)}}">

[nodir, 2017/06/13 02:10:14]

use [[]] for read-only data bindings

[cwpayton, 2017/06/13 16:22:14]

Done.

+          <template is="dom-repeat" items="[[searchList]]" as="config">
+            <config-card name="[[config.config_set]]"></config-card>
+          </template>
+        </template>
+      </template>
+    </div>
+  </template>
+
+  <script>
+    Polymer({
+      is: 'front-page',
+
+      properties: {
+        configList: {
+          type: Array,
+          value: []
+        },
+
+        isEmpty: {
+          type: Boolean,
+          value: true

[nodir, 2017/06/13 02:10:15]

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()

[cwpayton, 2017/06/13 16:22:14]

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."

[nodir, 2017/06/13 16:48:22]

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

[cwpayton, 2017/06/13 17:00:03]

Done.

+        },
+
+        isLoading: {
+          type: Boolean,
+          value: true
+        },
+
+        search: {

[nodir, 2017/06/13 02:10:15]

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".

[cwpayton, 2017/06/13 16:22:15]

Done.

+          type: String,
+          observer: '_searched'
+        },
+
+        searchList: {
+          type: Array,
+          value: []

[nodir, 2017/06/13 02:10:14]

see
https://www.polymer-project.org/1.0/docs/devguide/properties#configure-values
this should use a function that returns []

[cwpayton, 2017/06/13 16:22:15]

Done. Does it matter whether I use arrow function notation for this?

[nodir, 2017/06/13 16:48:22]

arrow function is nice

[cwpayton, 2017/06/13 17:00:03]

Done.

+        },
+
+        url: {
+          type: String,
+          value: "https://luci-config.appspot.com/_ah/api/config/v1/config-sets"
+        }

[nodir, 2017/06/13 02:10:15]

i don't think this property is needed

[cwpayton, 2017/06/13 16:22:15]

Done.

+      },
+
+      _getConfigs: function(event) {

[nodir, 2017/06/13 02:10:15]

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"

[cwpayton, 2017/06/13 16:22:14]

Done.

+        this.configList = event.detail.response.config_sets;
+        this.searchList = this._searched();

[nodir, 2017/06/13 02:10:14]

_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

[cwpayton, 2017/06/13 16:22:14]

Done.

+        this.isLoading = false;
+        this.isEmpty = this.searchList.length === 0;
+      },
+
+      _not: function(bool) {
+        return (!(bool));
+      },
+
+      _searched: function() {

[nodir, 2017/06/13 02:10:14]

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

[cwpayton, 2017/06/13 16:22:14]

Done.

+        this.searchList = [];
+        this.configList.forEach((elem) => {

[nodir, 2017/06/13 02:10:15]

parens around elem are unnecessary

[cwpayton, 2017/06/13 16:22:14]

Done.

+          if (elem.config_set.includes(this.search)) {
+            this.searchList.push(elem);
+          }
+        });

[nodir, 2017/06/13 02:10:14]

this essentially filters this.configList. Using filter method would be simpler
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...

[cwpayton, 2017/06/13 16:22:15]

Done.

+        this.isEmpty = this.searchList.length === 0;
+        return this.searchList;

[nodir, 2017/06/13 02:10:14]

there is no need to return this.searchList. If someone needs it, they can read
the list from this.searchList after calling this function

[cwpayton, 2017/06/13 16:22:15]

Done.

+      }
+    });
+  </script>
+</dom-module>