Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2921983002:
Swarming UI: Bold requested dimensions in bot dimensions (Closed)
DescriptionSwarming UI: Bold requested dimensions in bot dimensions
The bot dimensions that were requested by the task are now highlighted and bolded.
BUG=727947
Review-Url: https://codereview.chromium.org/2921983002
Committed: https://github.com/luci/luci-py/commit/86635eacdc145826e9f014cebb9233e488fb5914
Patch Set 1 #
Total comments: 19
Patch Set 2 : Swarming UI: Bold requested dimensions in bot dimensions #
Total comments: 1
Patch Set 3 : Regenerated vulcanized files and reverted package.json #Patch Set 4 : Deleted package-lock.json file #
Total comments: 18
Patch Set 5 : Edited task-page.html as per the code review comments #
Messages
Total messages: 44 (27 generated)
The CQ bit was checked by cwpayton@google.com to run a CQ dry run
Dry run: 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
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-infra-committers". Note that this has nothing to do with OWNERS files.
hinoka@google.com changed reviewers: + hinoka@chromium.org, hinoka@google.com
You'll want to fix your title and description first. You do that on rietveld by pressing "edit issue" on the left side. Title should be something like <Thing that changed, "swarming ui" in this case>: <1 line description> Description: <Longer description> On the bottom it should be: BUG=727947
Description was changed from ========== Merge branch 'master' of https://chromium.googlesource.com/external/github.com/luci/luci-py into requested_dimensions_bug Fixed the requested dimensions bug. First bug fix attempt BUG=The swarming UI now highlights and bolds the dimensions of the bots that were requested by the task ========== to ========== The bot dimensions that were requested by the task are now highlighted and bolded. BUG=727947 ==========
sergeyberezin@google.com changed reviewers: + sergeyberezin@chromium.org
I left my comments in https://chromium-review.googlesource.com/c/523382/ - I realized it was Gerrit way too late :-)
Looking good! For the title, something like: "Swarming UI: Bold requested dimensions in bot dimensions" And also copy that line in the first line of the description, since that becomes the title in the resulting commit. So the description should be === Swarming UI: Bold requested dimensions in bot dimensions The bot dimensions that were requested by the task are now highlighted and bolded. === In addition, it helps if you have a demo link or screenshot of the thing you changed, and include it as a message in the code review tool. https://codereview.chromium.org/2921983002/diff/1/appengine/swarming/ui/build... File appengine/swarming/ui/build/js/js.js (right): https://codereview.chromium.org/2921983002/diff/1/appengine/swarming/ui/build... appengine/swarming/ui/build/js/js.js:1: <<<<<<< HEAD This looks like a merge conflict, you'll need to delete this and regenerate it. https://codereview.chromium.org/2921983002/diff/1/appengine/swarming/ui/res/i... File appengine/swarming/ui/res/imp/taskpage/task-page.html (right): https://codereview.chromium.org/2921983002/diff/1/appengine/swarming/ui/res/i... appengine/swarming/ui/res/imp/taskpage/task-page.html:491: <b>[[dimension.key]]:</b> | <!-- Leave this divider in for readability --> Put divider and comment on new line for 80 char https://codereview.chromium.org/2921983002/diff/1/appengine/swarming/ui/res/i... appengine/swarming/ui/res/imp/taskpage/task-page.html:492: <template is="dom-repeat" items="[[_aliasResult(_request.properties.dimensions, dimension)]]" as="value"> <template is="..." items="...." as="...."> for 80 chars. https://codereview.chromium.org/2921983002/diff/1/appengine/swarming/ui/res/i... appengine/swarming/ui/res/imp/taskpage/task-page.html:493: <span class$="[[value.class]]">[[value.name]]</span> | <!-- Leave this divider in for readability --> Divider and comment on new line. https://codereview.chromium.org/2921983002/diff/1/appengine/swarming/ui/res/i... appengine/swarming/ui/res/imp/taskpage/task-page.html:749: // Refactor of the old "_alias" function for distinction from the You don't have to put in code comments for code changes, since the reader is only going to have the view of the code in a slice of time, so "old _alias" doesn't make much sense. For these types of comments, you usually leave them in the code review tool. https://codereview.chromium.org/2921983002/diff/1/appengine/swarming/ui/res/i... appengine/swarming/ui/res/imp/taskpage/task-page.html:767: // of request to determine whether the current bot dimension was requested. nit: 80 chars. https://codereview.chromium.org/2921983002/diff/1/appengine/swarming/ui/res/i... appengine/swarming/ui/res/imp/taskpage/task-page.html:768: var map = []; nit: this isn't a map, it's an array of objects. Maybe just call it "dimensions" https://codereview.chromium.org/2921983002/diff/1/appengine/swarming/ui/res/i... appengine/swarming/ui/res/imp/taskpage/task-page.html:779: values[i] = swarming.alias.has(result.key) ? nit: this is two lines, so just use an if/else statement here instead of a ternary statement. https://codereview.chromium.org/2921983002/diff/1/appengine/swarming/ui/res/i... appengine/swarming/ui/res/imp/taskpage/task-page.html:784: bold = target != null && request[target].value == v ? "bolded" : ""; This is a bit hard to parse, if statements are preferred. var bold = "" if (target != null && request[target].value == v) bold = "bolded" https://codereview.chromium.org/2921983002/diff/1/appengine/swarming/ui/res/i... appengine/swarming/ui/res/imp/taskpage/task-page.html:921: request.forEach(function (dim){ nit: space before {
Description was changed from ========== The bot dimensions that were requested by the task are now highlighted and bolded. BUG=727947 ========== to ========== Swarming UI: Bold requested dimensions in bot dimensions The bot dimensions that were requested by the task are now highlighted and bolded. BUG=727947 ==========
The CQ bit was checked by cwpayton@google.com to run a CQ dry run
Dry run: 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
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-infra-committers". Note that this has nothing to do with OWNERS files.
sergeyberezin@google.com changed reviewers: + sergeyberezin@google.com
The vulcanized filed shouldn't be deleted in this CL; they should only be regenerated.
https://codereview.chromium.org/2921983002/diff/10001/appengine/swarming/ui/p... File appengine/swarming/ui/package.json (right): https://codereview.chromium.org/2921983002/diff/10001/appengine/swarming/ui/p... appengine/swarming/ui/package.json:10: "html-minifier": "^3.0.3", Let's revert these changes in package.json - I don't believe you need the new versions. If new versions are indeed necessary, let's roll them in a separate CL, so they can be easily reverted.
Screenshots of the changes: https://screenshot.googleplex.com/EH41oVT5gWY https://codereview.chromium.org/2921983002/diff/1/appengine/swarming/ui/res/i... File appengine/swarming/ui/res/imp/taskpage/task-page.html (right): https://codereview.chromium.org/2921983002/diff/1/appengine/swarming/ui/res/i... appengine/swarming/ui/res/imp/taskpage/task-page.html:491: <b>[[dimension.key]]:</b> | <!-- Leave this divider in for readability --> On 2017/06/05 21:46:50, Ryan Tseng wrote: > Put divider and comment on new line for 80 char Done. https://codereview.chromium.org/2921983002/diff/1/appengine/swarming/ui/res/i... appengine/swarming/ui/res/imp/taskpage/task-page.html:492: <template is="dom-repeat" items="[[_aliasResult(_request.properties.dimensions, dimension)]]" as="value"> On 2017/06/05 21:46:53, Ryan Tseng wrote: > <template > is="..." > items="...." > as="...."> > > for 80 chars. Done. https://codereview.chromium.org/2921983002/diff/1/appengine/swarming/ui/res/i... appengine/swarming/ui/res/imp/taskpage/task-page.html:493: <span class$="[[value.class]]">[[value.name]]</span> | <!-- Leave this divider in for readability --> On 2017/06/05 21:46:52, Ryan Tseng wrote: > Divider and comment on new line. Done. https://codereview.chromium.org/2921983002/diff/1/appengine/swarming/ui/res/i... appengine/swarming/ui/res/imp/taskpage/task-page.html:749: // Refactor of the old "_alias" function for distinction from the On 2017/06/05 21:46:51, Ryan Tseng wrote: > You don't have to put in code comments for code changes, since the reader is > only going to have the view of the code in a slice of time, so "old _alias" > doesn't make much sense. For these types of comments, you usually leave them in > the code review tool. > Done. https://codereview.chromium.org/2921983002/diff/1/appengine/swarming/ui/res/i... appengine/swarming/ui/res/imp/taskpage/task-page.html:767: // of request to determine whether the current bot dimension was requested. On 2017/06/05 21:46:50, Ryan Tseng wrote: > nit: 80 chars. Done. https://codereview.chromium.org/2921983002/diff/1/appengine/swarming/ui/res/i... appengine/swarming/ui/res/imp/taskpage/task-page.html:768: var map = []; On 2017/06/05 21:46:50, Ryan Tseng wrote: > nit: this isn't a map, it's an array of objects. Maybe just call it > "dimensions" Done. https://codereview.chromium.org/2921983002/diff/1/appengine/swarming/ui/res/i... appengine/swarming/ui/res/imp/taskpage/task-page.html:779: values[i] = swarming.alias.has(result.key) ? On 2017/06/05 21:46:51, Ryan Tseng wrote: > nit: this is two lines, so just use an if/else statement here instead of a > ternary statement. Done. https://codereview.chromium.org/2921983002/diff/1/appengine/swarming/ui/res/i... appengine/swarming/ui/res/imp/taskpage/task-page.html:784: bold = target != null && request[target].value == v ? "bolded" : ""; On 2017/06/05 21:46:51, Ryan Tseng wrote: > This is a bit hard to parse, if statements are preferred. > > var bold = "" > if (target != null && request[target].value == v) bold = "bolded" Done. https://codereview.chromium.org/2921983002/diff/1/appengine/swarming/ui/res/i... appengine/swarming/ui/res/imp/taskpage/task-page.html:921: request.forEach(function (dim){ On 2017/06/05 21:46:51, Ryan Tseng wrote: > nit: space before { Done.
PTAL
Do we need package-lock.json file? It wasn't there before, let's not include it in the CL. The vulcanized files still show as deleted on the CL. Please make sure they are modified, not deleted.
The CQ bit was checked by sergeyberezin@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
LGTM + one last nit! Thank you! https://codereview.chromium.org/2921983002/diff/40001/appengine/swarming/ui/r... File appengine/swarming/ui/res/imp/taskpage/task-page.html (right): https://codereview.chromium.org/2921983002/diff/40001/appengine/swarming/ui/r... appengine/swarming/ui/res/imp/taskpage/task-page.html:769: // request is the entire _request.properties.dimensions array, but we are nit: please reformat to fit 80 chars.
hinoka@google.com changed reviewers: + kjlubick@chromium.org
lgtm + comments. Also wait for kjlubick's review. https://codereview.chromium.org/2921983002/diff/40001/appengine/swarming/ui/r... File appengine/swarming/ui/res/imp/taskpage/task-page.html (right): https://codereview.chromium.org/2921983002/diff/40001/appengine/swarming/ui/r... appengine/swarming/ui/res/imp/taskpage/task-page.html:136: span~span:before { This should be restricted to certain classes. ie span.dim~span.dim
kjlubick@google.com changed reviewers: + kjlubick@google.com - kjlubick@chromium.org
lgtm once nits are addressed. https://codereview.chromium.org/2921983002/diff/40001/appengine/swarming/ui/r... File appengine/swarming/ui/res/imp/taskpage/task-page.html (right): https://codereview.chromium.org/2921983002/diff/40001/appengine/swarming/ui/r... appengine/swarming/ui/res/imp/taskpage/task-page.html:494: <td bgcolor$="[[_highlight(_request.properties.dimensions, dimension.key)]]"> Super minor nit: you only need the $= for certain attributes like href, hidden, etc. Otherwise, just use = (aka a property binding) https://www.polymer-project.org/1.0/docs/devguide/data-binding#native-binding https://codereview.chromium.org/2921983002/diff/40001/appengine/swarming/ui/r... appengine/swarming/ui/res/imp/taskpage/task-page.html:768: _aliasResult: function(request, result) { Please rename request -> request_dims and result -> dim or similar Elsewhere request means the entire request object and result means the result object; we would prefer to not confuse the reader. https://codereview.chromium.org/2921983002/diff/40001/appengine/swarming/ui/r... appengine/swarming/ui/res/imp/taskpage/task-page.html:776: if (dim.key == result.key) target = i; Use triple equals unless there is a good reason not to (and then document what that good reason is). Format the if clause onto its own line, with {} https://codereview.chromium.org/2921983002/diff/40001/appengine/swarming/ui/r... appengine/swarming/ui/res/imp/taskpage/task-page.html:779: if (!Array.isArray(values)) { Just curious, is this logic just for safety or is the dimension's value occasionally not an array? https://codereview.chromium.org/2921983002/diff/40001/appengine/swarming/ui/r... appengine/swarming/ui/res/imp/taskpage/task-page.html:790: if (target != null && request[target].value == v) bold = "bolded" Format the if clause onto its own line, with {} https://codereview.chromium.org/2921983002/diff/40001/appengine/swarming/ui/r... appengine/swarming/ui/res/imp/taskpage/task-page.html:796: return dimensions; Minor naming suggestion (take it or leave it), dimensions -> boldMap https://codereview.chromium.org/2921983002/diff/40001/appengine/swarming/ui/r... appengine/swarming/ui/res/imp/taskpage/task-page.html:928: if (dim.key == key) highlight = true; Format the if clause onto its own line, with {}
PTAL https://codereview.chromium.org/2921983002/diff/40001/appengine/swarming/ui/r... File appengine/swarming/ui/res/imp/taskpage/task-page.html (right): https://codereview.chromium.org/2921983002/diff/40001/appengine/swarming/ui/r... appengine/swarming/ui/res/imp/taskpage/task-page.html:136: span~span:before { On 2017/06/06 00:27:55, Ryan Tseng wrote: > This should be restricted to certain classes. ie span.dim~span.dim Done. https://codereview.chromium.org/2921983002/diff/40001/appengine/swarming/ui/r... appengine/swarming/ui/res/imp/taskpage/task-page.html:494: <td bgcolor$="[[_highlight(_request.properties.dimensions, dimension.key)]]"> On 2017/06/06 12:26:54, kjlubick wrote: > Super minor nit: you only need the $= for certain attributes like href, hidden, > etc. Otherwise, just use = (aka a property binding) > > https://www.polymer-project.org/1.0/docs/devguide/data-binding#native-binding Turns out bgcolor needs a $= instead of a simple =. I made the change and the highlight stopped working, so I switched it back to $=. https://codereview.chromium.org/2921983002/diff/40001/appengine/swarming/ui/r... appengine/swarming/ui/res/imp/taskpage/task-page.html:768: _aliasResult: function(request, result) { On 2017/06/06 12:26:54, kjlubick wrote: > Please rename request -> request_dims and result -> dim or similar > > Elsewhere request means the entire request object and result means the result > object; we would prefer to not confuse the reader. Done. https://codereview.chromium.org/2921983002/diff/40001/appengine/swarming/ui/r... appengine/swarming/ui/res/imp/taskpage/task-page.html:769: // request is the entire _request.properties.dimensions array, but we are On 2017/06/06 00:14:05, Sergey Berezin wrote: > nit: please reformat to fit 80 chars. Done. https://codereview.chromium.org/2921983002/diff/40001/appengine/swarming/ui/r... appengine/swarming/ui/res/imp/taskpage/task-page.html:776: if (dim.key == result.key) target = i; On 2017/06/06 12:26:54, kjlubick wrote: > Use triple equals unless there is a good reason not to (and then document what > that good reason is). > > Format the if clause onto its own line, with {} Done. https://codereview.chromium.org/2921983002/diff/40001/appengine/swarming/ui/r... appengine/swarming/ui/res/imp/taskpage/task-page.html:779: if (!Array.isArray(values)) { On 2017/06/06 12:26:54, kjlubick wrote: > Just curious, is this logic just for safety or is the dimension's value > occasionally not an array? This check was written by someone else before me, so I assume they ran into a problem where if result only had one value, that value was simply passed in as-is instead of inside of an array, so I left it in. https://codereview.chromium.org/2921983002/diff/40001/appengine/swarming/ui/r... appengine/swarming/ui/res/imp/taskpage/task-page.html:790: if (target != null && request[target].value == v) bold = "bolded" On 2017/06/06 12:26:54, kjlubick wrote: > Format the if clause onto its own line, with {} Done. https://codereview.chromium.org/2921983002/diff/40001/appengine/swarming/ui/r... appengine/swarming/ui/res/imp/taskpage/task-page.html:796: return dimensions; On 2017/06/06 12:26:54, kjlubick wrote: > Minor naming suggestion (take it or leave it), dimensions -> boldMap Done. https://codereview.chromium.org/2921983002/diff/40001/appengine/swarming/ui/r... appengine/swarming/ui/res/imp/taskpage/task-page.html:928: if (dim.key == key) highlight = true; On 2017/06/06 12:26:54, kjlubick wrote: > Format the if clause onto its own line, with {} Done.
The CQ bit was checked by cwpayton@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyberezin@chromium.org, hinoka@google.com, kjlubick@google.com Link to the patchset: https://codereview.chromium.org/2921983002/#ps60001 (title: "Edited task-page.html as per the code review comments")
The CQ bit was unchecked by cwpayton@google.com
The CQ bit was checked by cwpayton@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 cwpayton@google.com
The CQ bit was checked by cwpayton@google.com
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": 60001, "attempt_start_ts": 1496771991720980,
"parent_rev": "fb80da79ce1c2ec9a85053576f6f182ab79b6c2a", "commit_rev":
"86635eacdc145826e9f014cebb9233e488fb5914"}
Message was sent while issue was closed.
Description was changed from ========== Swarming UI: Bold requested dimensions in bot dimensions The bot dimensions that were requested by the task are now highlighted and bolded. BUG=727947 ========== to ========== Swarming UI: Bold requested dimensions in bot dimensions The bot dimensions that were requested by the task are now highlighted and bolded. BUG=727947 Review-Url: https://codereview.chromium.org/2921983002 Committed: https://github.com/luci/luci-py/commit/86635eacdc145826e9f014cebb9233e488fb5914 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:60001) as https://github.com/luci/luci-py/commit/86635eacdc145826e9f014cebb9233e488fb5914
Message was sent while issue was closed.
On 2017/06/06 18:03:08, commit-bot: I haz the power wrote: > Committed patchset #5 (id:60001) as > https://github.com/luci/luci-py/commit/86635eacdc145826e9f014cebb9233e488fb5914 IMHO the highlight is a too intense; while this highlight is valuable when looking at the bot dimensions to see which ones were important, in the overall scheme of things, this is not the most important thing on the page. With the bright yellow full width background color, this immediately take attention on page load. In practice, this is not what the user wants to look at most of the time; this has local value (inside the Bot Dimensions box to better understand the relative importance of each value) but not overall value (with regards to the importance of other elements in the page). Please tone it down so it doesn't jump to the eye that much. Thanks.
Message was sent while issue was closed.
On 2017/06/08 at 13:56:41, maruel wrote: > Please tone it down so it doesn't jump to the eye that much. Thanks. The fix is in http://crrev.com/2930813003 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
