Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2249143002:
Make TaskList use Dynamic List (Closed)
DescriptionMake TaskList use Dynamic List
Stub out other components like filters
This is all very similar to Bot List, except there are no devices "sub table".
BUG=631047
Committed: https://github.com/luci/luci-py/commit/0be43acb22a4041ff898aad8d77c7ebcf04c358a
Patch Set 1 #Patch Set 2 : Document that filters is a stub #
Total comments: 20
Patch Set 3 : Address comments #Dependent Patchsets: Messages
Total messages: 27 (9 generated)
Description was changed from ========== Make TaskList use Dynamic List Stub out other components like filters BUG=631047 ========== to ========== Make TaskList use Dynamic List Stub out other components like filters This is all very similar to Bot List, except there are no devices "sub table". BUG=631047 ==========
kjlubick@google.com changed reviewers: + jcgregorio@google.com, stephana@google.com
Needs a screenshot of task-list-demo.html. https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... File appengine/swarming/elements/res/imp/tasklist/task-filters.html (right): https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... appengine/swarming/elements/res/imp/tasklist/task-filters.html:14: // outputs Stray comment in a comment? https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... appengine/swarming/elements/res/imp/tasklist/task-filters.html:45: // output stray comment? https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... File appengine/swarming/elements/res/imp/tasklist/task-list.html (right): https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... appengine/swarming/elements/res/imp/tasklist/task-list.html:17: client_id: String, will be set by server-side template evaluation. Comment should explain what the value is. How it is set, if it needs to be documented, can be done in public_tasklist_index.html. https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... appengine/swarming/elements/res/imp/tasklist/task-list.html:56: no blank line here. https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... appengine/swarming/elements/res/imp/tasklist/task-list.html:66: no blank line here. https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... appengine/swarming/elements/res/imp/tasklist/task-list.html:74: No blank line here. https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... appengine/swarming/elements/res/imp/tasklist/task-list.html:113: <template is="dom-repeat" If you are going to break the element across lines then line up all the attrs: <template is="dom-repeat" items="[[_plainColumns]]" as="c"> Check other instances, like tasks_table below. https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... appengine/swarming/elements/res/imp/tasklist/task-list.html:173: behaviors: [SwarmingBehaviors.SwarmingBehavior, behaviors: [ SwarmingBehaviors.SwarmingBehavior, SwarmingBehaviors.DynamicTableBehavior ], https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... appengine/swarming/elements/res/imp/tasklist/task-list.html:182: // for dynamic table Comments are sentences, or at least sentence-ish. // For dynamic table. https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... appengine/swarming/elements/res/imp/tasklist/task-list.html:199: no blank line here.
Screenshot: https://screenshot.googleplex.com/cz7iWS9T4tD Current demo: http://localhost:9050/res/imp/tasklist/task-list-demo.html?sort=id%3Aasc https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... File appengine/swarming/elements/res/imp/tasklist/task-filters.html (right): https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... appengine/swarming/elements/res/imp/tasklist/task-filters.html:14: // outputs On 2016/08/16 at 16:07:36, jcgregorio wrote: > Stray comment in a comment? There will be inputs. I usually like to list properties grouped by inputs and outputs. https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... appengine/swarming/elements/res/imp/tasklist/task-filters.html:45: // output On 2016/08/16 at 16:07:36, jcgregorio wrote: > stray comment? Same thing, see above. https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... File appengine/swarming/elements/res/imp/tasklist/task-list.html (right): https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... appengine/swarming/elements/res/imp/tasklist/task-list.html:17: client_id: String, will be set by server-side template evaluation. On 2016/08/16 at 16:07:36, jcgregorio wrote: > Comment should explain what the value is. How it is set, if it needs to be documented, can be done in public_tasklist_index.html. Done. How to set it is documented at https://github.com/luci/luci-py/blob/d030a87795a5bacfadaf09883350679ee0886a0f... https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... appengine/swarming/elements/res/imp/tasklist/task-list.html:56: On 2016/08/16 at 16:07:36, jcgregorio wrote: > no blank line here. Done. https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... appengine/swarming/elements/res/imp/tasklist/task-list.html:66: On 2016/08/16 at 16:07:36, jcgregorio wrote: > no blank line here. Done. https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... appengine/swarming/elements/res/imp/tasklist/task-list.html:74: On 2016/08/16 at 16:07:36, jcgregorio wrote: > No blank line here. Done. https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... appengine/swarming/elements/res/imp/tasklist/task-list.html:113: <template is="dom-repeat" On 2016/08/16 at 16:07:36, jcgregorio wrote: > If you are going to break the element across lines then line up all the attrs: > > <template > is="dom-repeat" > items="[[_plainColumns]]" > as="c"> > > Check other instances, like tasks_table below. Done. Also in bot-list.html https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... appengine/swarming/elements/res/imp/tasklist/task-list.html:173: behaviors: [SwarmingBehaviors.SwarmingBehavior, On 2016/08/16 at 16:07:36, jcgregorio wrote: > behaviors: [ > SwarmingBehaviors.SwarmingBehavior, > SwarmingBehaviors.DynamicTableBehavior > ], Done. https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... appengine/swarming/elements/res/imp/tasklist/task-list.html:182: // for dynamic table On 2016/08/16 at 16:07:36, jcgregorio wrote: > Comments are sentences, or at least sentence-ish. > > // For dynamic table. Done. https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... appengine/swarming/elements/res/imp/tasklist/task-list.html:199: On 2016/08/16 at 16:07:36, jcgregorio wrote: > no blank line here. Done.
On 2016/08/16 at 16:47:42, kjlubick wrote: > Screenshot: > https://screenshot.googleplex.com/cz7iWS9T4tD Needs more vertical space between titlebar and table: https://screenshot.googleplex.com/kYiY2Q0ARBm.png I would also suggest moving the Task Name to be the right-most column, so if you are on a narrow screen you will still see Status and Requesting User. > > Current demo: > http://localhost:9050/res/imp/tasklist/task-list-demo.html?sort=id%3Aasc > > https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... > File appengine/swarming/elements/res/imp/tasklist/task-filters.html (right): > > https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... > appengine/swarming/elements/res/imp/tasklist/task-filters.html:14: // outputs > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > Stray comment in a comment? > > There will be inputs. I usually like to list properties grouped by inputs and outputs. > > https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... > appengine/swarming/elements/res/imp/tasklist/task-filters.html:45: // output > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > stray comment? > > Same thing, see above. > > https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... > File appengine/swarming/elements/res/imp/tasklist/task-list.html (right): > > https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... > appengine/swarming/elements/res/imp/tasklist/task-list.html:17: client_id: String, will be set by server-side template evaluation. > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > Comment should explain what the value is. How it is set, if it needs to be documented, can be done in public_tasklist_index.html. > > Done. How to set it is documented at https://github.com/luci/luci-py/blob/d030a87795a5bacfadaf09883350679ee0886a0f... > > https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... > appengine/swarming/elements/res/imp/tasklist/task-list.html:56: > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > no blank line here. > > Done. > > https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... > appengine/swarming/elements/res/imp/tasklist/task-list.html:66: > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > no blank line here. > > Done. > > https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... > appengine/swarming/elements/res/imp/tasklist/task-list.html:74: > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > No blank line here. > > Done. > > https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... > appengine/swarming/elements/res/imp/tasklist/task-list.html:113: <template is="dom-repeat" > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > If you are going to break the element across lines then line up all the attrs: > > > > <template > > is="dom-repeat" > > items="[[_plainColumns]]" > > as="c"> > > > > Check other instances, like tasks_table below. > > Done. Also in bot-list.html > > https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... > appengine/swarming/elements/res/imp/tasklist/task-list.html:173: behaviors: [SwarmingBehaviors.SwarmingBehavior, > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > behaviors: [ > > SwarmingBehaviors.SwarmingBehavior, > > SwarmingBehaviors.DynamicTableBehavior > > ], > > Done. > > https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... > appengine/swarming/elements/res/imp/tasklist/task-list.html:182: // for dynamic table > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > Comments are sentences, or at least sentence-ish. > > > > // For dynamic table. > > Done. > > https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... > appengine/swarming/elements/res/imp/tasklist/task-list.html:199: > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > no blank line here. > > Done.
The way to filter the table will go where that arrow is pointing. I'll hold off on adjusting space until it's in (future CL) Task name seems so strange on the right. On narrow screens, I think it still looks fine because the columns automatically narrow: https://screenshot.googleplex.com/hvu1SVuAMJ1 On Thu, Aug 18, 2016 at 2:00 PM, <jcgregorio@google.com> wrote: > On 2016/08/16 at 16:47:42, kjlubick wrote: > > Screenshot: > > https://screenshot.googleplex.com/cz7iWS9T4tD > > Needs more vertical space between titlebar and table: > https://screenshot.googleplex.com/kYiY2Q0ARBm.png > > > I would also suggest moving the Task Name to be the right-most column, so > if you > are on a narrow screen you will still see Status and Requesting User. > > > > > > Current demo: > > http://localhost:9050/res/imp/tasklist/task-list-demo.html?sort=id%3Aasc > > > > > https://codereview.chromium.org/2249143002/diff/20001/ > appengine/swarming/elements/res/imp/tasklist/task-filters.html > > File appengine/swarming/elements/res/imp/tasklist/task-filters.html > (right): > > > > > https://codereview.chromium.org/2249143002/diff/20001/ > appengine/swarming/elements/res/imp/tasklist/task-filters.html#newcode14 > > appengine/swarming/elements/res/imp/tasklist/task-filters.html:14: // > outputs > > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > > Stray comment in a comment? > > > > There will be inputs. I usually like to list properties grouped by > inputs and > outputs. > > > > > https://codereview.chromium.org/2249143002/diff/20001/ > appengine/swarming/elements/res/imp/tasklist/task-filters.html#newcode45 > > appengine/swarming/elements/res/imp/tasklist/task-filters.html:45: // > output > > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > > stray comment? > > > > Same thing, see above. > > > > > https://codereview.chromium.org/2249143002/diff/20001/ > appengine/swarming/elements/res/imp/tasklist/task-list.html > > File appengine/swarming/elements/res/imp/tasklist/task-list.html > (right): > > > > > https://codereview.chromium.org/2249143002/diff/20001/ > appengine/swarming/elements/res/imp/tasklist/task-list.html#newcode17 > > appengine/swarming/elements/res/imp/tasklist/task-list.html:17: > client_id: > String, will be set by server-side template evaluation. > > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > > Comment should explain what the value is. How it is set, if it needs > to be > documented, can be done in public_tasklist_index.html. > > > > Done. How to set it is documented at > https://github.com/luci/luci-py/blob/d030a87795a5bacfadaf0988335067 > 9ee0886a0f/appengine/swarming/proto/config.proto#L37 > > > > > https://codereview.chromium.org/2249143002/diff/20001/ > appengine/swarming/elements/res/imp/tasklist/task-list.html#newcode56 > > appengine/swarming/elements/res/imp/tasklist/task-list.html:56: > > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > > no blank line here. > > > > Done. > > > > > https://codereview.chromium.org/2249143002/diff/20001/ > appengine/swarming/elements/res/imp/tasklist/task-list.html#newcode66 > > appengine/swarming/elements/res/imp/tasklist/task-list.html:66: > > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > > no blank line here. > > > > Done. > > > > > https://codereview.chromium.org/2249143002/diff/20001/ > appengine/swarming/elements/res/imp/tasklist/task-list.html#newcode74 > > appengine/swarming/elements/res/imp/tasklist/task-list.html:74: > > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > > No blank line here. > > > > Done. > > > > > https://codereview.chromium.org/2249143002/diff/20001/ > appengine/swarming/elements/res/imp/tasklist/task-list.html#newcode113 > > appengine/swarming/elements/res/imp/tasklist/task-list.html:113: > <template > is="dom-repeat" > > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > > If you are going to break the element across lines then line up all the > attrs: > > > > > > <template > > > is="dom-repeat" > > > items="[[_plainColumns]]" > > > as="c"> > > > > > > Check other instances, like tasks_table below. > > > > Done. Also in bot-list.html > > > > > https://codereview.chromium.org/2249143002/diff/20001/ > appengine/swarming/elements/res/imp/tasklist/task-list.html#newcode173 > > appengine/swarming/elements/res/imp/tasklist/task-list.html:173: > behaviors: > [SwarmingBehaviors.SwarmingBehavior, > > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > > behaviors: [ > > > SwarmingBehaviors.SwarmingBehavior, > > > SwarmingBehaviors.DynamicTableBehavior > > > ], > > > > Done. > > > > > https://codereview.chromium.org/2249143002/diff/20001/ > appengine/swarming/elements/res/imp/tasklist/task-list.html#newcode182 > > appengine/swarming/elements/res/imp/tasklist/task-list.html:182: // for > dynamic table > > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > > Comments are sentences, or at least sentence-ish. > > > > > > // For dynamic table. > > > > Done. > > > > > https://codereview.chromium.org/2249143002/diff/20001/ > appengine/swarming/elements/res/imp/tasklist/task-list.html#newcode199 > > appengine/swarming/elements/res/imp/tasklist/task-list.html:199: > > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > > no blank line here. > > > > Done. > > > > https://codereview.chromium.org/2249143002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/08/18 18:00:28, jcgregorio wrote: > On 2016/08/16 at 16:47:42, kjlubick wrote: > > Screenshot: > > https://screenshot.googleplex.com/cz7iWS9T4tD > > Needs more vertical space between titlebar and table: > https://screenshot.googleplex.com/kYiY2Q0ARBm.png > > > I would also suggest moving the Task Name to be the right-most column, so if you > are on a narrow screen you will still see Status and Requesting User. > > > > > Current demo: > > http://localhost:9050/res/imp/tasklist/task-list-demo.html?sort=id%3Aasc > > > > > https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... > > File appengine/swarming/elements/res/imp/tasklist/task-filters.html (right): > > > > > https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... > > appengine/swarming/elements/res/imp/tasklist/task-filters.html:14: // outputs > > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > > Stray comment in a comment? > > > > There will be inputs. I usually like to list properties grouped by inputs and > outputs. > > > > > https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... > > appengine/swarming/elements/res/imp/tasklist/task-filters.html:45: // output > > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > > stray comment? > > > > Same thing, see above. > > > > > https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... > > File appengine/swarming/elements/res/imp/tasklist/task-list.html (right): > > > > > https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... > > appengine/swarming/elements/res/imp/tasklist/task-list.html:17: client_id: > String, will be set by server-side template evaluation. > > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > > Comment should explain what the value is. How it is set, if it needs to be > documented, can be done in public_tasklist_index.html. > > > > Done. How to set it is documented at > https://github.com/luci/luci-py/blob/d030a87795a5bacfadaf09883350679ee0886a0f... > > > > > https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... > > appengine/swarming/elements/res/imp/tasklist/task-list.html:56: > > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > > no blank line here. > > > > Done. > > > > > https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... > > appengine/swarming/elements/res/imp/tasklist/task-list.html:66: > > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > > no blank line here. > > > > Done. > > > > > https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... > > appengine/swarming/elements/res/imp/tasklist/task-list.html:74: > > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > > No blank line here. > > > > Done. > > > > > https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... > > appengine/swarming/elements/res/imp/tasklist/task-list.html:113: <template > is="dom-repeat" > > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > > If you are going to break the element across lines then line up all the > attrs: > > > > > > <template > > > is="dom-repeat" > > > items="[[_plainColumns]]" > > > as="c"> > > > > > > Check other instances, like tasks_table below. > > > > Done. Also in bot-list.html > > > > > https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... > > appengine/swarming/elements/res/imp/tasklist/task-list.html:173: behaviors: > [SwarmingBehaviors.SwarmingBehavior, > > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > > behaviors: [ > > > SwarmingBehaviors.SwarmingBehavior, > > > SwarmingBehaviors.DynamicTableBehavior > > > ], > > > > Done. > > > > > https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... > > appengine/swarming/elements/res/imp/tasklist/task-list.html:182: // for > dynamic table > > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > > Comments are sentences, or at least sentence-ish. > > > > > > // For dynamic table. > > > > Done. > > > > > https://codereview.chromium.org/2249143002/diff/20001/appengine/swarming/elem... > > appengine/swarming/elements/res/imp/tasklist/task-list.html:199: > > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > > no blank line here. > > > > Done. Just a general comment. I find declarative requests to the backend very hard to reason about and counter intuitive. Now that we have the common.js package available via npm, we should consider using it for requests (sk.get, sk.post etc.) to the backend in all our code. It would make it easier if somebody else wanted to make changes.
On 2016/08/18 at 19:09:16, stephana wrote: > Just a general comment. I find declarative requests to the backend very hard to reason about and counter intuitive. > Now that we have the common.js package available via npm, we should consider using it for requests (sk.get, sk.post etc.) > to the backend in all our code. It would make it easier if somebody else wanted to make changes. +1
On 2016/08/18 at 18:57:41, chromium-reviews wrote: > The way to filter the table will go where that arrow is pointing. I'll > hold off on adjusting space until it's in (future CL) > > Task name seems so strange on the right. On narrow screens, I think it > still looks fine because the columns automatically narrow: > https://screenshot.googleplex.com/hvu1SVuAMJ1 Agreed, as long as the Task Name wraps, that looks fine. > > On Thu, Aug 18, 2016 at 2:00 PM, <jcgregorio@google.com> wrote: > > > On 2016/08/16 at 16:47:42, kjlubick wrote: > > > Screenshot: > > > https://screenshot.googleplex.com/cz7iWS9T4tD > > > > Needs more vertical space between titlebar and table: > > https://screenshot.googleplex.com/kYiY2Q0ARBm.png > > > > > > I would also suggest moving the Task Name to be the right-most column, so > > if you > > are on a narrow screen you will still see Status and Requesting User. > > > > > > > > > > Current demo: > > > http://localhost:9050/res/imp/tasklist/task-list-demo.html?sort=id%3Aasc > > > > > > > > https://codereview.chromium.org/2249143002/diff/20001/ > > appengine/swarming/elements/res/imp/tasklist/task-filters.html > > > File appengine/swarming/elements/res/imp/tasklist/task-filters.html > > (right): > > > > > > > > https://codereview.chromium.org/2249143002/diff/20001/ > > appengine/swarming/elements/res/imp/tasklist/task-filters.html#newcode14 > > > appengine/swarming/elements/res/imp/tasklist/task-filters.html:14: // > > outputs > > > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > > > Stray comment in a comment? > > > > > > There will be inputs. I usually like to list properties grouped by > > inputs and > > outputs. > > > > > > > > https://codereview.chromium.org/2249143002/diff/20001/ > > appengine/swarming/elements/res/imp/tasklist/task-filters.html#newcode45 > > > appengine/swarming/elements/res/imp/tasklist/task-filters.html:45: // > > output > > > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > > > stray comment? > > > > > > Same thing, see above. > > > > > > > > https://codereview.chromium.org/2249143002/diff/20001/ > > appengine/swarming/elements/res/imp/tasklist/task-list.html > > > File appengine/swarming/elements/res/imp/tasklist/task-list.html > > (right): > > > > > > > > https://codereview.chromium.org/2249143002/diff/20001/ > > appengine/swarming/elements/res/imp/tasklist/task-list.html#newcode17 > > > appengine/swarming/elements/res/imp/tasklist/task-list.html:17: > > client_id: > > String, will be set by server-side template evaluation. > > > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > > > Comment should explain what the value is. How it is set, if it needs > > to be > > documented, can be done in public_tasklist_index.html. > > > > > > Done. How to set it is documented at > > https://github.com/luci/luci-py/blob/d030a87795a5bacfadaf0988335067 > > 9ee0886a0f/appengine/swarming/proto/config.proto#L37 > > > > > > > > https://codereview.chromium.org/2249143002/diff/20001/ > > appengine/swarming/elements/res/imp/tasklist/task-list.html#newcode56 > > > appengine/swarming/elements/res/imp/tasklist/task-list.html:56: > > > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > > > no blank line here. > > > > > > Done. > > > > > > > > https://codereview.chromium.org/2249143002/diff/20001/ > > appengine/swarming/elements/res/imp/tasklist/task-list.html#newcode66 > > > appengine/swarming/elements/res/imp/tasklist/task-list.html:66: > > > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > > > no blank line here. > > > > > > Done. > > > > > > > > https://codereview.chromium.org/2249143002/diff/20001/ > > appengine/swarming/elements/res/imp/tasklist/task-list.html#newcode74 > > > appengine/swarming/elements/res/imp/tasklist/task-list.html:74: > > > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > > > No blank line here. > > > > > > Done. > > > > > > > > https://codereview.chromium.org/2249143002/diff/20001/ > > appengine/swarming/elements/res/imp/tasklist/task-list.html#newcode113 > > > appengine/swarming/elements/res/imp/tasklist/task-list.html:113: > > <template > > is="dom-repeat" > > > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > > > If you are going to break the element across lines then line up all the > > attrs: > > > > > > > > <template > > > > is="dom-repeat" > > > > items="[[_plainColumns]]" > > > > as="c"> > > > > > > > > Check other instances, like tasks_table below. > > > > > > Done. Also in bot-list.html > > > > > > > > https://codereview.chromium.org/2249143002/diff/20001/ > > appengine/swarming/elements/res/imp/tasklist/task-list.html#newcode173 > > > appengine/swarming/elements/res/imp/tasklist/task-list.html:173: > > behaviors: > > [SwarmingBehaviors.SwarmingBehavior, > > > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > > > behaviors: [ > > > > SwarmingBehaviors.SwarmingBehavior, > > > > SwarmingBehaviors.DynamicTableBehavior > > > > ], > > > > > > Done. > > > > > > > > https://codereview.chromium.org/2249143002/diff/20001/ > > appengine/swarming/elements/res/imp/tasklist/task-list.html#newcode182 > > > appengine/swarming/elements/res/imp/tasklist/task-list.html:182: // for > > dynamic table > > > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > > > Comments are sentences, or at least sentence-ish. > > > > > > > > // For dynamic table. > > > > > > Done. > > > > > > > > https://codereview.chromium.org/2249143002/diff/20001/ > > appengine/swarming/elements/res/imp/tasklist/task-list.html#newcode199 > > > appengine/swarming/elements/res/imp/tasklist/task-list.html:199: > > > On 2016/08/16 at 16:07:36, jcgregorio wrote: > > > > no blank line here. > > > > > > Done. > > > > > > > > https://codereview.chromium.org/2249143002/ > > > > -- > You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. >
On 2016/08/18 at 19:09:16, stephana wrote: > > Just a general comment. I find declarative requests to the backend very hard to reason about and counter intuitive. > Now that we have the common.js package available via npm, we should consider using it for requests (sk.get, sk.post etc.) > to the backend in all our code. It would make it easier if somebody else wanted to make changes. Okay. We can discuss this more at length when I get back and look into changing to using sk.request (as we need to control headers and params).
lgtm
kjlubick@chromium.org changed reviewers: + kjlubick@chromium.org
lgtm
The CQ bit was checked by kjlubick@chromium.org
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/30bc5e0f278f5910)
The CQ bit was checked by kjlubick@google.com
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/30bc90723b557010)
The CQ bit was checked by kjlubick@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/19 17:30:48, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... lgtm
Message was sent while issue was closed.
Description was changed from ========== Make TaskList use Dynamic List Stub out other components like filters This is all very similar to Bot List, except there are no devices "sub table". BUG=631047 ========== to ========== Make TaskList use Dynamic List Stub out other components like filters This is all very similar to Bot List, except there are no devices "sub table". BUG=631047 Committed: https://github.com/luci/luci-py/commit/0be43acb22a4041ff898aad8d77c7ebcf04c358a ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://github.com/luci/luci-py/commit/0be43acb22a4041ff898aad8d77c7ebcf04c358a |
