Re: Review Request 21247: Add config grouping visualisation to job page
On May 10, 2014, 2:14 a.m., Suman Karumuri wrote:
src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js,
line 385
https://reviews.apache.org/r/21247/diff/1/?file=577123#file577123line385
please only pass the required fields into the fields into the function.
By passing in $scope, it is very unclear what inputs buildGroupSummary
depends on.
David McLaughlin wrote:
The function signature is quite clear that buildGroupSummary is a
function which depends on the global $scope (it both reads and writes to it -
this is how Angular passes around such objects to its controllers which do
the same thing).
Just to be clear on this point. Take a look at getTasksForJob:
function getTasksForJob(role, environment, job) {
...
}
You might think this gives some form of clarity, but it does not. It's
confusing, because this function goes on to use the following variables via a
closure:
auroraClient, $scope, taskUtil, getJobDashboardUrl, summarizeTask
On the other hand, everything that buildGroupSummary needs is passed into the
function or created inside it. You could copy and paste this function elsewhere
in the code base - move it completely outside of the controller or even pass it
into the controller via DI and the call to this function _from the controller_
would not change. I think ultimately all these functions used to group code
together need to be moved and this controller simplified, but I want to keep
this review small and do the larger cleaning up in another ticket.
- David
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21247/#review42626
---
On May 8, 2014, 11:45 p.m., David McLaughlin wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21247/
---
(Updated May 8, 2014, 11:45 p.m.)
Review request for Aurora, Suman Karumuri and Bill Farner.
Bugs: AURORA-378
https://issues.apache.org/jira/browse/AURORA-378
Repository: aurora
Description
---
Replace the button to show/hide configs with a bar when there are multiple.
Tidied up the display of the configuration data.
Diffs
-
src/main/java/org/apache/aurora/scheduler/http/ServletModule.java
983101277ffbd1c4017b29f4c86e61315f1bcc78
src/main/resources/org/apache/aurora/scheduler/http/ui/configSummary.html
PRE-CREATION
src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css
dc515ac818a9af36522bb07a125cd92ff9230fa2
src/main/resources/org/apache/aurora/scheduler/http/ui/groupSummary.html
PRE-CREATION
src/main/resources/org/apache/aurora/scheduler/http/ui/job.html
cd78c20bee7891a6cdfd19943a6f7e8a9dce33df
src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js
7c80bd769b7e80bef0822846166959925b1bf7f8
src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js
a79276d2486fc1122a8b084ea0614cdae759f88c
Diff: https://reviews.apache.org/r/21247/diff/
Testing
---
File Attachments
Job page with only one config group
https://reviews.apache.org/media/uploaded/files/2014/05/08/908570f2-bef2-4370-bf08-1b5c7400c3c6__Screen_Shot_2014-05-08_at_4.26.02_PM.png
Showing config for only one config group
https://reviews.apache.org/media/uploaded/files/2014/05/08/1424f2c3-cf66-4ce4-a595-dc3f7647d6d2__Screen_Shot_2014-05-08_at_4.26.12_PM.png
Job page with multiple config groups
https://reviews.apache.org/media/uploaded/files/2014/05/08/bc9c1616-3c71-4bcc-ab40-fefc56d5e19a__Screen_Shot_2014-05-08_at_4.25.21_PM.png
Viewing one of the groups from config bar
https://reviews.apache.org/media/uploaded/files/2014/05/08/6b8e6077-c8c8-48c9-94e9-732e782204b6__Screen_Shot_2014-05-08_at_4.25.36_PM.png
Viewing multiple configs at the same time
https://reviews.apache.org/media/uploaded/files/2014/05/08/b7b2cba1-034b-43e6-9eaa-3ba1a78f87f5__Screen_Shot_2014-05-08_at_4.25.42_PM.png
Thanks,
David McLaughlin
Re: Review Request 21247: Add config grouping visualisation to job page
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21247/ --- (Updated May 15, 2014, 9:47 p.m.) Review request for Aurora, Suman Karumuri and Bill Farner. Changes --- rebase Bugs: AURORA-378 https://issues.apache.org/jira/browse/AURORA-378 Repository: aurora Description --- Replace the button to show/hide configs with a bar when there are multiple. Tidied up the display of the configuration data. Diffs (updated) - src/main/java/org/apache/aurora/scheduler/http/ServletModule.java effd48a95da459f92ed0f38a7bc35fe9e33b774a src/main/resources/org/apache/aurora/scheduler/http/ui/configSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css d64c5b4788040577c285c9f928f9ca520b19e449 src/main/resources/org/apache/aurora/scheduler/http/ui/groupSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/job.html 284c2bcefb420c6a59ab029bc547248bae727e36 src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js c13a88f3ee1f5975bc6c29dca2e5a12ef27e024e src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js a79276d2486fc1122a8b084ea0614cdae759f88c Diff: https://reviews.apache.org/r/21247/diff/ Testing --- File Attachments Job page with only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/908570f2-bef2-4370-bf08-1b5c7400c3c6__Screen_Shot_2014-05-08_at_4.26.02_PM.png Showing config for only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/1424f2c3-cf66-4ce4-a595-dc3f7647d6d2__Screen_Shot_2014-05-08_at_4.26.12_PM.png Job page with multiple config groups https://reviews.apache.org/media/uploaded/files/2014/05/08/bc9c1616-3c71-4bcc-ab40-fefc56d5e19a__Screen_Shot_2014-05-08_at_4.25.21_PM.png Viewing one of the groups from config bar https://reviews.apache.org/media/uploaded/files/2014/05/08/6b8e6077-c8c8-48c9-94e9-732e782204b6__Screen_Shot_2014-05-08_at_4.25.36_PM.png Viewing multiple configs at the same time https://reviews.apache.org/media/uploaded/files/2014/05/08/b7b2cba1-034b-43e6-9eaa-3ba1a78f87f5__Screen_Shot_2014-05-08_at_4.25.42_PM.png screenshot of really small group https://reviews.apache.org/media/uploaded/files/2014/05/12/2d7999ac-ba1f-45b0-bf9e-99f9c7a10009__Screen_Shot_2014-05-12_at_1.27.59_PM.png Thanks, David McLaughlin
Re: Review Request 21247: Add config grouping visualisation to job page
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21247/#review43180 --- This diff is merged. Please close out this review and the associated ticket. - Suman Karumuri On May 15, 2014, 9:47 p.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21247/ --- (Updated May 15, 2014, 9:47 p.m.) Review request for Aurora, Suman Karumuri and Bill Farner. Bugs: AURORA-378 https://issues.apache.org/jira/browse/AURORA-378 Repository: aurora Description --- Replace the button to show/hide configs with a bar when there are multiple. Tidied up the display of the configuration data. Diffs - src/main/java/org/apache/aurora/scheduler/http/ServletModule.java effd48a95da459f92ed0f38a7bc35fe9e33b774a src/main/resources/org/apache/aurora/scheduler/http/ui/configSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css d64c5b4788040577c285c9f928f9ca520b19e449 src/main/resources/org/apache/aurora/scheduler/http/ui/groupSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/job.html 284c2bcefb420c6a59ab029bc547248bae727e36 src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js c13a88f3ee1f5975bc6c29dca2e5a12ef27e024e src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js a79276d2486fc1122a8b084ea0614cdae759f88c Diff: https://reviews.apache.org/r/21247/diff/ Testing --- File Attachments Job page with only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/908570f2-bef2-4370-bf08-1b5c7400c3c6__Screen_Shot_2014-05-08_at_4.26.02_PM.png Showing config for only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/1424f2c3-cf66-4ce4-a595-dc3f7647d6d2__Screen_Shot_2014-05-08_at_4.26.12_PM.png Job page with multiple config groups https://reviews.apache.org/media/uploaded/files/2014/05/08/bc9c1616-3c71-4bcc-ab40-fefc56d5e19a__Screen_Shot_2014-05-08_at_4.25.21_PM.png Viewing one of the groups from config bar https://reviews.apache.org/media/uploaded/files/2014/05/08/6b8e6077-c8c8-48c9-94e9-732e782204b6__Screen_Shot_2014-05-08_at_4.25.36_PM.png Viewing multiple configs at the same time https://reviews.apache.org/media/uploaded/files/2014/05/08/b7b2cba1-034b-43e6-9eaa-3ba1a78f87f5__Screen_Shot_2014-05-08_at_4.25.42_PM.png screenshot of really small group https://reviews.apache.org/media/uploaded/files/2014/05/12/2d7999ac-ba1f-45b0-bf9e-99f9c7a10009__Screen_Shot_2014-05-12_at_1.27.59_PM.png Thanks, David McLaughlin
Re: Review Request 21247: Add config grouping visualisation to job page
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21247/#review42980 --- Ship it! Ship It! - Suman Karumuri On May 14, 2014, 3:46 p.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21247/ --- (Updated May 14, 2014, 3:46 p.m.) Review request for Aurora, Suman Karumuri and Bill Farner. Bugs: AURORA-378 https://issues.apache.org/jira/browse/AURORA-378 Repository: aurora Description --- Replace the button to show/hide configs with a bar when there are multiple. Tidied up the display of the configuration data. Diffs - src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 983101277ffbd1c4017b29f4c86e61315f1bcc78 src/main/resources/org/apache/aurora/scheduler/http/ui/configSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css dc515ac818a9af36522bb07a125cd92ff9230fa2 src/main/resources/org/apache/aurora/scheduler/http/ui/groupSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/job.html cd78c20bee7891a6cdfd19943a6f7e8a9dce33df src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 7c80bd769b7e80bef0822846166959925b1bf7f8 src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js a79276d2486fc1122a8b084ea0614cdae759f88c Diff: https://reviews.apache.org/r/21247/diff/ Testing --- File Attachments Job page with only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/908570f2-bef2-4370-bf08-1b5c7400c3c6__Screen_Shot_2014-05-08_at_4.26.02_PM.png Showing config for only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/1424f2c3-cf66-4ce4-a595-dc3f7647d6d2__Screen_Shot_2014-05-08_at_4.26.12_PM.png Job page with multiple config groups https://reviews.apache.org/media/uploaded/files/2014/05/08/bc9c1616-3c71-4bcc-ab40-fefc56d5e19a__Screen_Shot_2014-05-08_at_4.25.21_PM.png Viewing one of the groups from config bar https://reviews.apache.org/media/uploaded/files/2014/05/08/6b8e6077-c8c8-48c9-94e9-732e782204b6__Screen_Shot_2014-05-08_at_4.25.36_PM.png Viewing multiple configs at the same time https://reviews.apache.org/media/uploaded/files/2014/05/08/b7b2cba1-034b-43e6-9eaa-3ba1a78f87f5__Screen_Shot_2014-05-08_at_4.25.42_PM.png screenshot of really small group https://reviews.apache.org/media/uploaded/files/2014/05/12/2d7999ac-ba1f-45b0-bf9e-99f9c7a10009__Screen_Shot_2014-05-12_at_1.27.59_PM.png Thanks, David McLaughlin
Re: Review Request 21247: Add config grouping visualisation to job page
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21247/#review42626 --- I think we should show the config bar even when there is one config. It will also act a visual indication to the user that all his tasks are in a consistent config. File Attachment: Viewing multiple configs at the same time - Screen Shot 2014-05-08 at 4.25.42 PM.png https://reviews.apache.org//r/21247/#fcomment7 What happens when there are more than 2 configs? Will these configs wrap around? please add a close option to divs so the users can hide the configs they don't want. That will let users compare the different configs all at once. src/main/resources/org/apache/aurora/scheduler/http/ui/configSummary.html https://reviews.apache.org/r/21247/#comment76448 wrap this in a div. src/main/resources/org/apache/aurora/scheduler/http/ui/groupSummary.html https://reviews.apache.org/r/21247/#comment76449 Please make this an abbr and add the label to the tooltip also. If there are too many different configs or if the label is too long, the user would still be able to look at it. src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js https://reviews.apache.org/r/21247/#comment76451 please only pass the required fields into the fields into the function. By passing in $scope, it is very unclear what inputs buildGroupSummary depends on. src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js https://reviews.apache.org/r/21247/#comment76453 please move all the code related to group summary into it's own controller. That will make the code modular and easily testable. src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js https://reviews.apache.org/r/21247/#comment76452 Please initialize all the new fields to scope outside this function. As is, a typo in a variable name will cause the UI to render incorrectly and it would be really hard to debug what went wrong. - Suman Karumuri On May 8, 2014, 11:45 p.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21247/ --- (Updated May 8, 2014, 11:45 p.m.) Review request for Aurora, Suman Karumuri and Bill Farner. Bugs: AURORA-378 https://issues.apache.org/jira/browse/AURORA-378 Repository: aurora Description --- Replace the button to show/hide configs with a bar when there are multiple. Tidied up the display of the configuration data. Diffs - src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 983101277ffbd1c4017b29f4c86e61315f1bcc78 src/main/resources/org/apache/aurora/scheduler/http/ui/configSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css dc515ac818a9af36522bb07a125cd92ff9230fa2 src/main/resources/org/apache/aurora/scheduler/http/ui/groupSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/job.html cd78c20bee7891a6cdfd19943a6f7e8a9dce33df src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 7c80bd769b7e80bef0822846166959925b1bf7f8 src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js a79276d2486fc1122a8b084ea0614cdae759f88c Diff: https://reviews.apache.org/r/21247/diff/ Testing --- File Attachments Job page with only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/908570f2-bef2-4370-bf08-1b5c7400c3c6__Screen_Shot_2014-05-08_at_4.26.02_PM.png Showing config for only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/1424f2c3-cf66-4ce4-a595-dc3f7647d6d2__Screen_Shot_2014-05-08_at_4.26.12_PM.png Job page with multiple config groups https://reviews.apache.org/media/uploaded/files/2014/05/08/bc9c1616-3c71-4bcc-ab40-fefc56d5e19a__Screen_Shot_2014-05-08_at_4.25.21_PM.png Viewing one of the groups from config bar https://reviews.apache.org/media/uploaded/files/2014/05/08/6b8e6077-c8c8-48c9-94e9-732e782204b6__Screen_Shot_2014-05-08_at_4.25.36_PM.png Viewing multiple configs at the same time https://reviews.apache.org/media/uploaded/files/2014/05/08/b7b2cba1-034b-43e6-9eaa-3ba1a78f87f5__Screen_Shot_2014-05-08_at_4.25.42_PM.png Thanks, David McLaughlin
Re: Review Request 21247: Add config grouping visualisation to job page
On May 14, 2014, 4:26 a.m., Suman Karumuri wrote: src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, line 386 https://reviews.apache.org/r/21247/diff/5/?file=581259#file581259line386 I see this function call (function definition itself is good) as a potential land mine. For example, moving this function from line 358 to 356, would cause buildGroupSummary to break and this error will be very hard to debug. Your comment here is an indication that this needs special attention. Moving this function call into the getTasksForJob(line 398) function will obviate the need for this comment and will also make the code flow more straightforward. I agree that this controller code can use some refactoring, but I think this is a potential land mine and there is nothing to be gained from calling it outside the normal call path. Please feel free to leave a TODO to refactor getTasksForJob. buildGroupSummary needs $scope.taskSummary to be defined. If I move it to line 398, then every reason for this being 'a land mine' still exists. This is synchronous, imperative code. Why would it be surprising that you can't just move code around without breaking stuff? Also, it's not going to be hard to debug. The error will say Cannot call map on undefined and TypeError: Cannot read property 'map' of undefined at buildGroupSummary (http://localhost:8081/js/controllers.js:372:47) at new anonymous (http://localhost:8081/js/controllers.js:356:5) at invoke (http://localhost:8081/js/angular.js:3697:17) at Object.instantiate (http://localhost:8081/js/angular.js:3708:23) at http://localhost:8081/js/angular.js:6758:28 at link (http://localhost:8081/js/angular-route.js:906:26) at nodeLinkFn (http://localhost:8081/js/angular.js:6212:13) at compositeLinkFn (http://localhost:8081/js/angular.js:5622:15) at publicLinkFn (http://localhost:8081/js/angular.js:5527:30) at boundTranscludeFn (http://localhost:8081/js/angular.js:5641:21) ng-view class=ng-scope - David --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21247/#review42937 --- On May 14, 2014, 1:30 a.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21247/ --- (Updated May 14, 2014, 1:30 a.m.) Review request for Aurora, Suman Karumuri and Bill Farner. Bugs: AURORA-378 https://issues.apache.org/jira/browse/AURORA-378 Repository: aurora Description --- Replace the button to show/hide configs with a bar when there are multiple. Tidied up the display of the configuration data. Diffs - src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 983101277ffbd1c4017b29f4c86e61315f1bcc78 src/main/resources/org/apache/aurora/scheduler/http/ui/configSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css dc515ac818a9af36522bb07a125cd92ff9230fa2 src/main/resources/org/apache/aurora/scheduler/http/ui/groupSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/job.html cd78c20bee7891a6cdfd19943a6f7e8a9dce33df src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 7c80bd769b7e80bef0822846166959925b1bf7f8 src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js a79276d2486fc1122a8b084ea0614cdae759f88c Diff: https://reviews.apache.org/r/21247/diff/ Testing --- File Attachments Job page with only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/908570f2-bef2-4370-bf08-1b5c7400c3c6__Screen_Shot_2014-05-08_at_4.26.02_PM.png Showing config for only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/1424f2c3-cf66-4ce4-a595-dc3f7647d6d2__Screen_Shot_2014-05-08_at_4.26.12_PM.png Job page with multiple config groups https://reviews.apache.org/media/uploaded/files/2014/05/08/bc9c1616-3c71-4bcc-ab40-fefc56d5e19a__Screen_Shot_2014-05-08_at_4.25.21_PM.png Viewing one of the groups from config bar https://reviews.apache.org/media/uploaded/files/2014/05/08/6b8e6077-c8c8-48c9-94e9-732e782204b6__Screen_Shot_2014-05-08_at_4.25.36_PM.png Viewing multiple configs at the same time https://reviews.apache.org/media/uploaded/files/2014/05/08/b7b2cba1-034b-43e6-9eaa-3ba1a78f87f5__Screen_Shot_2014-05-08_at_4.25.42_PM.png screenshot of really small group https://reviews.apache.org/media/uploaded/files/2014/05/12/2d7999ac-ba1f-45b0-bf9e-99f9c7a10009__Screen_Shot_2014-05-12_at_1.27.59_PM.png Thanks, David McLaughlin
Re: Review Request 21247: Add config grouping visualisation to job page
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21247/ --- (Updated May 14, 2014, 3:46 p.m.) Review request for Aurora, Suman Karumuri and Bill Farner. Bugs: AURORA-378 https://issues.apache.org/jira/browse/AURORA-378 Repository: aurora Description --- Replace the button to show/hide configs with a bar when there are multiple. Tidied up the display of the configuration data. Diffs (updated) - src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 983101277ffbd1c4017b29f4c86e61315f1bcc78 src/main/resources/org/apache/aurora/scheduler/http/ui/configSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css dc515ac818a9af36522bb07a125cd92ff9230fa2 src/main/resources/org/apache/aurora/scheduler/http/ui/groupSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/job.html cd78c20bee7891a6cdfd19943a6f7e8a9dce33df src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 7c80bd769b7e80bef0822846166959925b1bf7f8 src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js a79276d2486fc1122a8b084ea0614cdae759f88c Diff: https://reviews.apache.org/r/21247/diff/ Testing --- File Attachments Job page with only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/908570f2-bef2-4370-bf08-1b5c7400c3c6__Screen_Shot_2014-05-08_at_4.26.02_PM.png Showing config for only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/1424f2c3-cf66-4ce4-a595-dc3f7647d6d2__Screen_Shot_2014-05-08_at_4.26.12_PM.png Job page with multiple config groups https://reviews.apache.org/media/uploaded/files/2014/05/08/bc9c1616-3c71-4bcc-ab40-fefc56d5e19a__Screen_Shot_2014-05-08_at_4.25.21_PM.png Viewing one of the groups from config bar https://reviews.apache.org/media/uploaded/files/2014/05/08/6b8e6077-c8c8-48c9-94e9-732e782204b6__Screen_Shot_2014-05-08_at_4.25.36_PM.png Viewing multiple configs at the same time https://reviews.apache.org/media/uploaded/files/2014/05/08/b7b2cba1-034b-43e6-9eaa-3ba1a78f87f5__Screen_Shot_2014-05-08_at_4.25.42_PM.png screenshot of really small group https://reviews.apache.org/media/uploaded/files/2014/05/12/2d7999ac-ba1f-45b0-bf9e-99f9c7a10009__Screen_Shot_2014-05-12_at_1.27.59_PM.png Thanks, David McLaughlin
Re: Review Request 21247: Add config grouping visualisation to job page
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21247/#review42981 --- Thanks for the changes. - Suman Karumuri On May 14, 2014, 3:46 p.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21247/ --- (Updated May 14, 2014, 3:46 p.m.) Review request for Aurora, Suman Karumuri and Bill Farner. Bugs: AURORA-378 https://issues.apache.org/jira/browse/AURORA-378 Repository: aurora Description --- Replace the button to show/hide configs with a bar when there are multiple. Tidied up the display of the configuration data. Diffs - src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 983101277ffbd1c4017b29f4c86e61315f1bcc78 src/main/resources/org/apache/aurora/scheduler/http/ui/configSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css dc515ac818a9af36522bb07a125cd92ff9230fa2 src/main/resources/org/apache/aurora/scheduler/http/ui/groupSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/job.html cd78c20bee7891a6cdfd19943a6f7e8a9dce33df src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 7c80bd769b7e80bef0822846166959925b1bf7f8 src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js a79276d2486fc1122a8b084ea0614cdae759f88c Diff: https://reviews.apache.org/r/21247/diff/ Testing --- File Attachments Job page with only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/908570f2-bef2-4370-bf08-1b5c7400c3c6__Screen_Shot_2014-05-08_at_4.26.02_PM.png Showing config for only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/1424f2c3-cf66-4ce4-a595-dc3f7647d6d2__Screen_Shot_2014-05-08_at_4.26.12_PM.png Job page with multiple config groups https://reviews.apache.org/media/uploaded/files/2014/05/08/bc9c1616-3c71-4bcc-ab40-fefc56d5e19a__Screen_Shot_2014-05-08_at_4.25.21_PM.png Viewing one of the groups from config bar https://reviews.apache.org/media/uploaded/files/2014/05/08/6b8e6077-c8c8-48c9-94e9-732e782204b6__Screen_Shot_2014-05-08_at_4.25.36_PM.png Viewing multiple configs at the same time https://reviews.apache.org/media/uploaded/files/2014/05/08/b7b2cba1-034b-43e6-9eaa-3ba1a78f87f5__Screen_Shot_2014-05-08_at_4.25.42_PM.png screenshot of really small group https://reviews.apache.org/media/uploaded/files/2014/05/12/2d7999ac-ba1f-45b0-bf9e-99f9c7a10009__Screen_Shot_2014-05-12_at_1.27.59_PM.png Thanks, David McLaughlin
Re: Review Request 21247: Add config grouping visualisation to job page
On May 10, 2014, 2:14 a.m., Suman Karumuri wrote: I think we should show the config bar even when there is one config. It will also act a visual indication to the user that all his tasks are in a consistent config. This is a fair suggestion, and I tried this initially. Having a 100% color bar across the header of the page dominates the user's attention when there is nothing for them to see. On May 10, 2014, 2:14 a.m., Suman Karumuri wrote: File Attachment: Viewing multiple configs at the same time - Screen Shot 2014-05-08 at 4.25.42 PM.png https://reviews.apache.org/r/21247/#fcomment8 What happens when there are more than 2 configs? Will these configs wrap around? please add a close option to divs so the users can hide the configs they don't want. That will let users compare the different configs all at once. Yes, see the CSS. It is a fluid floating list which will scale with the page width. If you look at the code, the bars are toggles to show/hide individual configs. On May 10, 2014, 2:14 a.m., Suman Karumuri wrote: src/main/resources/org/apache/aurora/scheduler/http/ui/configSummary.html, line 1 https://reviews.apache.org/r/21247/diff/1/?file=577119#file577119line1 wrap this in a div. Why? On May 10, 2014, 2:14 a.m., Suman Karumuri wrote: src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, line 387 https://reviews.apache.org/r/21247/diff/1/?file=577123#file577123line387 please move all the code related to group summary into it's own controller. That will make the code modular and easily testable. I think it's best to keep the code consistent with what is there already, and fix the design of the app in another ticket. On May 10, 2014, 2:14 a.m., Suman Karumuri wrote: src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, line 385 https://reviews.apache.org/r/21247/diff/1/?file=577123#file577123line385 please only pass the required fields into the fields into the function. By passing in $scope, it is very unclear what inputs buildGroupSummary depends on. The function signature is quite clear that buildGroupSummary is a function which depends on the global $scope (it both reads and writes to it - this is how Angular passes around such objects to its controllers which do the same thing). On May 10, 2014, 2:14 a.m., Suman Karumuri wrote: src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, line 396 https://reviews.apache.org/r/21247/diff/1/?file=577123#file577123line396 Please initialize all the new fields to scope outside this function. As is, a typo in a variable name will cause the UI to render incorrectly and it would be really hard to debug what went wrong. The function is always called, there is no need to initialize the fields outside of the function. Typos in variable names are always a source of bugs and whether the typo is in the parent scope or this one doesn't really change that. - David --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21247/#review42626 --- On May 8, 2014, 11:45 p.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21247/ --- (Updated May 8, 2014, 11:45 p.m.) Review request for Aurora, Suman Karumuri and Bill Farner. Bugs: AURORA-378 https://issues.apache.org/jira/browse/AURORA-378 Repository: aurora Description --- Replace the button to show/hide configs with a bar when there are multiple. Tidied up the display of the configuration data. Diffs - src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 983101277ffbd1c4017b29f4c86e61315f1bcc78 src/main/resources/org/apache/aurora/scheduler/http/ui/configSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css dc515ac818a9af36522bb07a125cd92ff9230fa2 src/main/resources/org/apache/aurora/scheduler/http/ui/groupSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/job.html cd78c20bee7891a6cdfd19943a6f7e8a9dce33df src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 7c80bd769b7e80bef0822846166959925b1bf7f8 src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js a79276d2486fc1122a8b084ea0614cdae759f88c Diff: https://reviews.apache.org/r/21247/diff/ Testing --- File Attachments Job page with only one config group
Re: Review Request 21247: Add config grouping visualisation to job page
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21247/ --- (Updated May 14, 2014, 1:30 a.m.) Review request for Aurora, Suman Karumuri and Bill Farner. Changes --- The config bar is now always on, Suman got a +1 on his viewpoint from a design resource in Seattle so I'll considered myself overruled. All of the interaction and event handlers for my directive have been moved inside the directive itself. The controller now only has code to munge the data into the correct format. Now any controller which wants to reuse this directive just needs to get the data into the same format - at which point we should move that logic into a service. Bugs: AURORA-378 https://issues.apache.org/jira/browse/AURORA-378 Repository: aurora Description --- Replace the button to show/hide configs with a bar when there are multiple. Tidied up the display of the configuration data. Diffs (updated) - src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 983101277ffbd1c4017b29f4c86e61315f1bcc78 src/main/resources/org/apache/aurora/scheduler/http/ui/configSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css dc515ac818a9af36522bb07a125cd92ff9230fa2 src/main/resources/org/apache/aurora/scheduler/http/ui/groupSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/job.html cd78c20bee7891a6cdfd19943a6f7e8a9dce33df src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 7c80bd769b7e80bef0822846166959925b1bf7f8 src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js a79276d2486fc1122a8b084ea0614cdae759f88c Diff: https://reviews.apache.org/r/21247/diff/ Testing --- File Attachments Job page with only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/908570f2-bef2-4370-bf08-1b5c7400c3c6__Screen_Shot_2014-05-08_at_4.26.02_PM.png Showing config for only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/1424f2c3-cf66-4ce4-a595-dc3f7647d6d2__Screen_Shot_2014-05-08_at_4.26.12_PM.png Job page with multiple config groups https://reviews.apache.org/media/uploaded/files/2014/05/08/bc9c1616-3c71-4bcc-ab40-fefc56d5e19a__Screen_Shot_2014-05-08_at_4.25.21_PM.png Viewing one of the groups from config bar https://reviews.apache.org/media/uploaded/files/2014/05/08/6b8e6077-c8c8-48c9-94e9-732e782204b6__Screen_Shot_2014-05-08_at_4.25.36_PM.png Viewing multiple configs at the same time https://reviews.apache.org/media/uploaded/files/2014/05/08/b7b2cba1-034b-43e6-9eaa-3ba1a78f87f5__Screen_Shot_2014-05-08_at_4.25.42_PM.png screenshot of really small group https://reviews.apache.org/media/uploaded/files/2014/05/12/2d7999ac-ba1f-45b0-bf9e-99f9c7a10009__Screen_Shot_2014-05-12_at_1.27.59_PM.png Thanks, David McLaughlin
Re: Review Request 21247: Add config grouping visualisation to job page
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21247/#review42937 --- This looks really good. Thanks for the changes. The only ship it blocker is the function call below. src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js https://reviews.apache.org/r/21247/#comment76879 I see this function call (function definition itself is good) as a potential land mine. For example, moving this function from line 358 to 356, would cause buildGroupSummary to break and this error will be very hard to debug. Your comment here is an indication that this needs special attention. Moving this function call into the getTasksForJob(line 398) function will obviate the need for this comment and will also make the code flow more straightforward. I agree that this controller code can use some refactoring, but I think this is a potential land mine and there is nothing to be gained from calling it outside the normal call path. Please feel free to leave a TODO to refactor getTasksForJob. - Suman Karumuri On May 14, 2014, 1:30 a.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21247/ --- (Updated May 14, 2014, 1:30 a.m.) Review request for Aurora, Suman Karumuri and Bill Farner. Bugs: AURORA-378 https://issues.apache.org/jira/browse/AURORA-378 Repository: aurora Description --- Replace the button to show/hide configs with a bar when there are multiple. Tidied up the display of the configuration data. Diffs - src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 983101277ffbd1c4017b29f4c86e61315f1bcc78 src/main/resources/org/apache/aurora/scheduler/http/ui/configSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css dc515ac818a9af36522bb07a125cd92ff9230fa2 src/main/resources/org/apache/aurora/scheduler/http/ui/groupSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/job.html cd78c20bee7891a6cdfd19943a6f7e8a9dce33df src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 7c80bd769b7e80bef0822846166959925b1bf7f8 src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js a79276d2486fc1122a8b084ea0614cdae759f88c Diff: https://reviews.apache.org/r/21247/diff/ Testing --- File Attachments Job page with only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/908570f2-bef2-4370-bf08-1b5c7400c3c6__Screen_Shot_2014-05-08_at_4.26.02_PM.png Showing config for only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/1424f2c3-cf66-4ce4-a595-dc3f7647d6d2__Screen_Shot_2014-05-08_at_4.26.12_PM.png Job page with multiple config groups https://reviews.apache.org/media/uploaded/files/2014/05/08/bc9c1616-3c71-4bcc-ab40-fefc56d5e19a__Screen_Shot_2014-05-08_at_4.25.21_PM.png Viewing one of the groups from config bar https://reviews.apache.org/media/uploaded/files/2014/05/08/6b8e6077-c8c8-48c9-94e9-732e782204b6__Screen_Shot_2014-05-08_at_4.25.36_PM.png Viewing multiple configs at the same time https://reviews.apache.org/media/uploaded/files/2014/05/08/b7b2cba1-034b-43e6-9eaa-3ba1a78f87f5__Screen_Shot_2014-05-08_at_4.25.42_PM.png screenshot of really small group https://reviews.apache.org/media/uploaded/files/2014/05/12/2d7999ac-ba1f-45b0-bf9e-99f9c7a10009__Screen_Shot_2014-05-12_at_1.27.59_PM.png Thanks, David McLaughlin
Re: Review Request 21247: Add config grouping visualisation to job page
On May 10, 2014, 2:14 a.m., Suman Karumuri wrote: src/main/resources/org/apache/aurora/scheduler/http/ui/groupSummary.html, line 6 https://reviews.apache.org/r/21247/diff/1/?file=577121#file577121line6 Please make this an abbr and add the label to the tooltip also. If there are too many different configs or if the label is too long, the user would still be able to look at it. Using abbr like this is not what that tag was designed for - it is for annotating abbreviations for screen-readers, search engine crawlers, etc. I'm going to add this here to keep our mouseovers consistent with the abbr tags you added, but let's move (back?) to bootstrap tooltips. - David --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21247/#review42626 --- On May 8, 2014, 11:45 p.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21247/ --- (Updated May 8, 2014, 11:45 p.m.) Review request for Aurora, Suman Karumuri and Bill Farner. Bugs: AURORA-378 https://issues.apache.org/jira/browse/AURORA-378 Repository: aurora Description --- Replace the button to show/hide configs with a bar when there are multiple. Tidied up the display of the configuration data. Diffs - src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 983101277ffbd1c4017b29f4c86e61315f1bcc78 src/main/resources/org/apache/aurora/scheduler/http/ui/configSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css dc515ac818a9af36522bb07a125cd92ff9230fa2 src/main/resources/org/apache/aurora/scheduler/http/ui/groupSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/job.html cd78c20bee7891a6cdfd19943a6f7e8a9dce33df src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 7c80bd769b7e80bef0822846166959925b1bf7f8 src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js a79276d2486fc1122a8b084ea0614cdae759f88c Diff: https://reviews.apache.org/r/21247/diff/ Testing --- File Attachments Job page with only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/908570f2-bef2-4370-bf08-1b5c7400c3c6__Screen_Shot_2014-05-08_at_4.26.02_PM.png Showing config for only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/1424f2c3-cf66-4ce4-a595-dc3f7647d6d2__Screen_Shot_2014-05-08_at_4.26.12_PM.png Job page with multiple config groups https://reviews.apache.org/media/uploaded/files/2014/05/08/bc9c1616-3c71-4bcc-ab40-fefc56d5e19a__Screen_Shot_2014-05-08_at_4.25.21_PM.png Viewing one of the groups from config bar https://reviews.apache.org/media/uploaded/files/2014/05/08/6b8e6077-c8c8-48c9-94e9-732e782204b6__Screen_Shot_2014-05-08_at_4.25.36_PM.png Viewing multiple configs at the same time https://reviews.apache.org/media/uploaded/files/2014/05/08/b7b2cba1-034b-43e6-9eaa-3ba1a78f87f5__Screen_Shot_2014-05-08_at_4.25.42_PM.png Thanks, David McLaughlin
Re: Review Request 21247: Add config grouping visualisation to job page
On May 9, 2014, 10:27 p.m., Bill Farner wrote: src/main/resources/org/apache/aurora/scheduler/http/ui/groupSummary.html, line 21 https://reviews.apache.org/r/21247/diff/1/?file=577121#file577121line21 kill extra newline done. On May 9, 2014, 10:27 p.m., Bill Farner wrote: src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, line 400 https://reviews.apache.org/r/21247/diff/1/?file=577123#file577123line400 s/if(/if (/ done. would be nice to have checkstyle for our JS code. - David --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21247/#review42617 --- On May 12, 2014, 6:50 p.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21247/ --- (Updated May 12, 2014, 6:50 p.m.) Review request for Aurora, Suman Karumuri and Bill Farner. Bugs: AURORA-378 https://issues.apache.org/jira/browse/AURORA-378 Repository: aurora Description --- Replace the button to show/hide configs with a bar when there are multiple. Tidied up the display of the configuration data. Diffs - src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 983101277ffbd1c4017b29f4c86e61315f1bcc78 src/main/resources/org/apache/aurora/scheduler/http/ui/configSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css dc515ac818a9af36522bb07a125cd92ff9230fa2 src/main/resources/org/apache/aurora/scheduler/http/ui/groupSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/job.html cd78c20bee7891a6cdfd19943a6f7e8a9dce33df src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 7c80bd769b7e80bef0822846166959925b1bf7f8 src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js a79276d2486fc1122a8b084ea0614cdae759f88c Diff: https://reviews.apache.org/r/21247/diff/ Testing --- File Attachments Job page with only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/908570f2-bef2-4370-bf08-1b5c7400c3c6__Screen_Shot_2014-05-08_at_4.26.02_PM.png Showing config for only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/1424f2c3-cf66-4ce4-a595-dc3f7647d6d2__Screen_Shot_2014-05-08_at_4.26.12_PM.png Job page with multiple config groups https://reviews.apache.org/media/uploaded/files/2014/05/08/bc9c1616-3c71-4bcc-ab40-fefc56d5e19a__Screen_Shot_2014-05-08_at_4.25.21_PM.png Viewing one of the groups from config bar https://reviews.apache.org/media/uploaded/files/2014/05/08/6b8e6077-c8c8-48c9-94e9-732e782204b6__Screen_Shot_2014-05-08_at_4.25.36_PM.png Viewing multiple configs at the same time https://reviews.apache.org/media/uploaded/files/2014/05/08/b7b2cba1-034b-43e6-9eaa-3ba1a78f87f5__Screen_Shot_2014-05-08_at_4.25.42_PM.png Thanks, David McLaughlin
Re: Review Request 21247: Add config grouping visualisation to job page
On May 9, 2014, 10:27 p.m., Bill Farner wrote: src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, line 400 https://reviews.apache.org/r/21247/diff/1/?file=577123#file577123line400 s/if(/if (/ David McLaughlin wrote: done. would be nice to have checkstyle for our JS code. Bill Farner wrote: I completely agree. Is that something you would like to champion? Sure, I'll try and add JSLint/JSHint build integration as part of AURORA-228. - David --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21247/#review42617 --- On May 12, 2014, 8:28 p.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21247/ --- (Updated May 12, 2014, 8:28 p.m.) Review request for Aurora, Suman Karumuri and Bill Farner. Bugs: AURORA-378 https://issues.apache.org/jira/browse/AURORA-378 Repository: aurora Description --- Replace the button to show/hide configs with a bar when there are multiple. Tidied up the display of the configuration data. Diffs - src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 983101277ffbd1c4017b29f4c86e61315f1bcc78 src/main/resources/org/apache/aurora/scheduler/http/ui/configSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css dc515ac818a9af36522bb07a125cd92ff9230fa2 src/main/resources/org/apache/aurora/scheduler/http/ui/groupSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/job.html cd78c20bee7891a6cdfd19943a6f7e8a9dce33df src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 7c80bd769b7e80bef0822846166959925b1bf7f8 src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js a79276d2486fc1122a8b084ea0614cdae759f88c Diff: https://reviews.apache.org/r/21247/diff/ Testing --- File Attachments Job page with only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/908570f2-bef2-4370-bf08-1b5c7400c3c6__Screen_Shot_2014-05-08_at_4.26.02_PM.png Showing config for only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/1424f2c3-cf66-4ce4-a595-dc3f7647d6d2__Screen_Shot_2014-05-08_at_4.26.12_PM.png Job page with multiple config groups https://reviews.apache.org/media/uploaded/files/2014/05/08/bc9c1616-3c71-4bcc-ab40-fefc56d5e19a__Screen_Shot_2014-05-08_at_4.25.21_PM.png Viewing one of the groups from config bar https://reviews.apache.org/media/uploaded/files/2014/05/08/6b8e6077-c8c8-48c9-94e9-732e782204b6__Screen_Shot_2014-05-08_at_4.25.36_PM.png Viewing multiple configs at the same time https://reviews.apache.org/media/uploaded/files/2014/05/08/b7b2cba1-034b-43e6-9eaa-3ba1a78f87f5__Screen_Shot_2014-05-08_at_4.25.42_PM.png screenshot of really small group https://reviews.apache.org/media/uploaded/files/2014/05/12/2d7999ac-ba1f-45b0-bf9e-99f9c7a10009__Screen_Shot_2014-05-12_at_1.27.59_PM.png Thanks, David McLaughlin
Re: Review Request 21247: Add config grouping visualisation to job page
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21247/ --- (Updated May 12, 2014, 6:50 p.m.) Review request for Aurora, Suman Karumuri and Bill Farner. Bugs: AURORA-378 https://issues.apache.org/jira/browse/AURORA-378 Repository: aurora Description --- Replace the button to show/hide configs with a bar when there are multiple. Tidied up the display of the configuration data. Diffs (updated) - src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 983101277ffbd1c4017b29f4c86e61315f1bcc78 src/main/resources/org/apache/aurora/scheduler/http/ui/configSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css dc515ac818a9af36522bb07a125cd92ff9230fa2 src/main/resources/org/apache/aurora/scheduler/http/ui/groupSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/job.html cd78c20bee7891a6cdfd19943a6f7e8a9dce33df src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 7c80bd769b7e80bef0822846166959925b1bf7f8 src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js a79276d2486fc1122a8b084ea0614cdae759f88c Diff: https://reviews.apache.org/r/21247/diff/ Testing --- File Attachments Job page with only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/908570f2-bef2-4370-bf08-1b5c7400c3c6__Screen_Shot_2014-05-08_at_4.26.02_PM.png Showing config for only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/1424f2c3-cf66-4ce4-a595-dc3f7647d6d2__Screen_Shot_2014-05-08_at_4.26.12_PM.png Job page with multiple config groups https://reviews.apache.org/media/uploaded/files/2014/05/08/bc9c1616-3c71-4bcc-ab40-fefc56d5e19a__Screen_Shot_2014-05-08_at_4.25.21_PM.png Viewing one of the groups from config bar https://reviews.apache.org/media/uploaded/files/2014/05/08/6b8e6077-c8c8-48c9-94e9-732e782204b6__Screen_Shot_2014-05-08_at_4.25.36_PM.png Viewing multiple configs at the same time https://reviews.apache.org/media/uploaded/files/2014/05/08/b7b2cba1-034b-43e6-9eaa-3ba1a78f87f5__Screen_Shot_2014-05-08_at_4.25.42_PM.png Thanks, David McLaughlin
Re: Review Request 21247: Add config grouping visualisation to job page
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21247/ --- (Updated May 12, 2014, 8:27 p.m.) Review request for Aurora, Suman Karumuri and Bill Farner. Changes --- Fixed overflow issue with the bar when the width of a group is too narrow. Removed need for abbr, by using an a tag with title attribute. Bugs: AURORA-378 https://issues.apache.org/jira/browse/AURORA-378 Repository: aurora Description --- Replace the button to show/hide configs with a bar when there are multiple. Tidied up the display of the configuration data. Diffs (updated) - src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 983101277ffbd1c4017b29f4c86e61315f1bcc78 src/main/resources/org/apache/aurora/scheduler/http/ui/configSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css dc515ac818a9af36522bb07a125cd92ff9230fa2 src/main/resources/org/apache/aurora/scheduler/http/ui/groupSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/job.html cd78c20bee7891a6cdfd19943a6f7e8a9dce33df src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 7c80bd769b7e80bef0822846166959925b1bf7f8 src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js a79276d2486fc1122a8b084ea0614cdae759f88c Diff: https://reviews.apache.org/r/21247/diff/ Testing --- File Attachments Job page with only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/908570f2-bef2-4370-bf08-1b5c7400c3c6__Screen_Shot_2014-05-08_at_4.26.02_PM.png Showing config for only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/1424f2c3-cf66-4ce4-a595-dc3f7647d6d2__Screen_Shot_2014-05-08_at_4.26.12_PM.png Job page with multiple config groups https://reviews.apache.org/media/uploaded/files/2014/05/08/bc9c1616-3c71-4bcc-ab40-fefc56d5e19a__Screen_Shot_2014-05-08_at_4.25.21_PM.png Viewing one of the groups from config bar https://reviews.apache.org/media/uploaded/files/2014/05/08/6b8e6077-c8c8-48c9-94e9-732e782204b6__Screen_Shot_2014-05-08_at_4.25.36_PM.png Viewing multiple configs at the same time https://reviews.apache.org/media/uploaded/files/2014/05/08/b7b2cba1-034b-43e6-9eaa-3ba1a78f87f5__Screen_Shot_2014-05-08_at_4.25.42_PM.png Thanks, David McLaughlin
Re: Review Request 21247: Add config grouping visualisation to job page
On May 10, 2014, 2:14 a.m., Suman Karumuri wrote: I think we should show the config bar even when there is one config. It will also act a visual indication to the user that all his tasks are in a consistent config. David McLaughlin wrote: This is a fair suggestion, and I tried this initially. Having a 100% color bar across the header of the page dominates the user's attention when there is nothing for them to see. I think the bar grabbing the user's attention when he goes to a page is ok in this case because we are telling him that all the jobs are having the same config. Once we add something else like a job status bar that needs user's attention, we can consider hiding this automatically. Further, when we add auto-refresh to the page, the bar would keep appearing and disappearing during an update which can be jarring. So, I think we should show the bar always. On May 10, 2014, 2:14 a.m., Suman Karumuri wrote: File Attachment: Viewing multiple configs at the same time - Screen Shot 2014-05-08 at 4.25.42 PM.png https://reviews.apache.org/r/21247/#fcomment9 What happens when there are more than 2 configs? Will these configs wrap around? please add a close option to divs so the users can hide the configs they don't want. That will let users compare the different configs all at once. David McLaughlin wrote: Yes, see the CSS. It is a fluid floating list which will scale with the page width. If you look at the code, the bars are toggles to show/hide individual configs. I couldn't test this code on my laptop so I am not sure of the interactions. Can we do something so we can test this locally without adding dummy data (ex: updating IsolatedSchedulerModule etc..)? On May 10, 2014, 2:14 a.m., Suman Karumuri wrote: src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, line 387 https://reviews.apache.org/r/21247/diff/1/?file=577123#file577123line387 please move all the code related to group summary into it's own controller. That will make the code modular and easily testable. David McLaughlin wrote: I think it's best to keep the code consistent with what is there already, and fix the design of the app in another ticket. There is no need for the JobController should know about how the summary data is presented. Please refactor it out into a separate controller. Also, please delete the existing task summary code from the controller. On May 10, 2014, 2:14 a.m., Suman Karumuri wrote: src/main/resources/org/apache/aurora/scheduler/http/ui/configSummary.html, line 1 https://reviews.apache.org/r/21247/diff/1/?file=577119#file577119line1 wrap this in a div. David McLaughlin wrote: Why? I can't find a reference now, but I read that all components should be wrapped in a div tag. Please ignore this if you don't think it's not required. On May 10, 2014, 2:14 a.m., Suman Karumuri wrote: src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, line 396 https://reviews.apache.org/r/21247/diff/1/?file=577123#file577123line396 Please initialize all the new fields to scope outside this function. As is, a typo in a variable name will cause the UI to render incorrectly and it would be really hard to debug what went wrong. David McLaughlin wrote: The function is always called, there is no need to initialize the fields outside of the function. Typos in variable names are always a source of bugs and whether the typo is in the parent scope or this one doesn't really change that. I still feel that declaring all your variables in the class before you use them is a good coding practice. That's how the rest of the code base is. Per a JS dev here, that is also considered a best practice. As is, I don't really know how many new variables you have added. I also agree that typos is a weak argument. On May 10, 2014, 2:14 a.m., Suman Karumuri wrote: src/main/resources/org/apache/aurora/scheduler/http/ui/groupSummary.html, line 6 https://reviews.apache.org/r/21247/diff/1/?file=577121#file577121line6 Please make this an abbr and add the label to the tooltip also. If there are too many different configs or if the label is too long, the user would still be able to look at it. David McLaughlin wrote: Using abbr like this is not what that tag was designed for - it is for annotating abbreviations for screen-readers, search engine crawlers, etc. I'm going to add this here to keep our mouseovers consistent with the abbr tags you added, but let's move (back?) to bootstrap tooltips. Good idea. We should move to using tooltips. On May 10, 2014, 2:14 a.m., Suman Karumuri wrote: src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, line 385 https://reviews.apache.org/r/21247/diff/1/?file=577123#file577123line385 please only pass the required
Re: Review Request 21247: Add config grouping visualisation to job page
On May 10, 2014, 2:14 a.m., Suman Karumuri wrote: src/main/resources/org/apache/aurora/scheduler/http/ui/configSummary.html, line 1 https://reviews.apache.org/r/21247/diff/1/?file=577119#file577119line1 wrap this in a div. David McLaughlin wrote: Why? Suman Karumuri wrote: I can't find a reference now, but I read that all components should be wrapped in a div tag. Please ignore this if you don't think it's not required. Yeah, I am ignoring this unless you can find that source.. it would mean you couldn't have any child elements where the parents are not divs as templates (e.g. an li, tr, td, etc.). - David --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21247/#review42626 --- On May 13, 2014, 12:02 a.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21247/ --- (Updated May 13, 2014, 12:02 a.m.) Review request for Aurora, Suman Karumuri and Bill Farner. Bugs: AURORA-378 https://issues.apache.org/jira/browse/AURORA-378 Repository: aurora Description --- Replace the button to show/hide configs with a bar when there are multiple. Tidied up the display of the configuration data. Diffs - src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 983101277ffbd1c4017b29f4c86e61315f1bcc78 src/main/resources/org/apache/aurora/scheduler/http/ui/configSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css dc515ac818a9af36522bb07a125cd92ff9230fa2 src/main/resources/org/apache/aurora/scheduler/http/ui/groupSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/job.html cd78c20bee7891a6cdfd19943a6f7e8a9dce33df src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 7c80bd769b7e80bef0822846166959925b1bf7f8 src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js a79276d2486fc1122a8b084ea0614cdae759f88c Diff: https://reviews.apache.org/r/21247/diff/ Testing --- File Attachments Job page with only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/908570f2-bef2-4370-bf08-1b5c7400c3c6__Screen_Shot_2014-05-08_at_4.26.02_PM.png Showing config for only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/1424f2c3-cf66-4ce4-a595-dc3f7647d6d2__Screen_Shot_2014-05-08_at_4.26.12_PM.png Job page with multiple config groups https://reviews.apache.org/media/uploaded/files/2014/05/08/bc9c1616-3c71-4bcc-ab40-fefc56d5e19a__Screen_Shot_2014-05-08_at_4.25.21_PM.png Viewing one of the groups from config bar https://reviews.apache.org/media/uploaded/files/2014/05/08/6b8e6077-c8c8-48c9-94e9-732e782204b6__Screen_Shot_2014-05-08_at_4.25.36_PM.png Viewing multiple configs at the same time https://reviews.apache.org/media/uploaded/files/2014/05/08/b7b2cba1-034b-43e6-9eaa-3ba1a78f87f5__Screen_Shot_2014-05-08_at_4.25.42_PM.png screenshot of really small group https://reviews.apache.org/media/uploaded/files/2014/05/12/2d7999ac-ba1f-45b0-bf9e-99f9c7a10009__Screen_Shot_2014-05-12_at_1.27.59_PM.png Thanks, David McLaughlin
Re: Review Request 21247: Add config grouping visualisation to job page
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21247/#review42759 --- Also, please think of a way to test this locally. src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js https://reviews.apache.org/r/21247/#comment76642 Please delete the code for the task summary table from the controller. - Suman Karumuri On May 12, 2014, 8:28 p.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21247/ --- (Updated May 12, 2014, 8:28 p.m.) Review request for Aurora, Suman Karumuri and Bill Farner. Bugs: AURORA-378 https://issues.apache.org/jira/browse/AURORA-378 Repository: aurora Description --- Replace the button to show/hide configs with a bar when there are multiple. Tidied up the display of the configuration data. Diffs - src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 983101277ffbd1c4017b29f4c86e61315f1bcc78 src/main/resources/org/apache/aurora/scheduler/http/ui/configSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css dc515ac818a9af36522bb07a125cd92ff9230fa2 src/main/resources/org/apache/aurora/scheduler/http/ui/groupSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/job.html cd78c20bee7891a6cdfd19943a6f7e8a9dce33df src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 7c80bd769b7e80bef0822846166959925b1bf7f8 src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js a79276d2486fc1122a8b084ea0614cdae759f88c Diff: https://reviews.apache.org/r/21247/diff/ Testing --- File Attachments Job page with only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/908570f2-bef2-4370-bf08-1b5c7400c3c6__Screen_Shot_2014-05-08_at_4.26.02_PM.png Showing config for only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/1424f2c3-cf66-4ce4-a595-dc3f7647d6d2__Screen_Shot_2014-05-08_at_4.26.12_PM.png Job page with multiple config groups https://reviews.apache.org/media/uploaded/files/2014/05/08/bc9c1616-3c71-4bcc-ab40-fefc56d5e19a__Screen_Shot_2014-05-08_at_4.25.21_PM.png Viewing one of the groups from config bar https://reviews.apache.org/media/uploaded/files/2014/05/08/6b8e6077-c8c8-48c9-94e9-732e782204b6__Screen_Shot_2014-05-08_at_4.25.36_PM.png Viewing multiple configs at the same time https://reviews.apache.org/media/uploaded/files/2014/05/08/b7b2cba1-034b-43e6-9eaa-3ba1a78f87f5__Screen_Shot_2014-05-08_at_4.25.42_PM.png screenshot of really small group https://reviews.apache.org/media/uploaded/files/2014/05/12/2d7999ac-ba1f-45b0-bf9e-99f9c7a10009__Screen_Shot_2014-05-12_at_1.27.59_PM.png Thanks, David McLaughlin
Re: Review Request 21247: Add config grouping visualisation to job page
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21247/ --- (Updated May 13, 2014, 12:02 a.m.) Review request for Aurora, Suman Karumuri and Bill Farner. Changes --- Removed old task summary UI code from controller. Bugs: AURORA-378 https://issues.apache.org/jira/browse/AURORA-378 Repository: aurora Description --- Replace the button to show/hide configs with a bar when there are multiple. Tidied up the display of the configuration data. Diffs (updated) - src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 983101277ffbd1c4017b29f4c86e61315f1bcc78 src/main/resources/org/apache/aurora/scheduler/http/ui/configSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css dc515ac818a9af36522bb07a125cd92ff9230fa2 src/main/resources/org/apache/aurora/scheduler/http/ui/groupSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/job.html cd78c20bee7891a6cdfd19943a6f7e8a9dce33df src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 7c80bd769b7e80bef0822846166959925b1bf7f8 src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js a79276d2486fc1122a8b084ea0614cdae759f88c Diff: https://reviews.apache.org/r/21247/diff/ Testing --- File Attachments Job page with only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/908570f2-bef2-4370-bf08-1b5c7400c3c6__Screen_Shot_2014-05-08_at_4.26.02_PM.png Showing config for only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/1424f2c3-cf66-4ce4-a595-dc3f7647d6d2__Screen_Shot_2014-05-08_at_4.26.12_PM.png Job page with multiple config groups https://reviews.apache.org/media/uploaded/files/2014/05/08/bc9c1616-3c71-4bcc-ab40-fefc56d5e19a__Screen_Shot_2014-05-08_at_4.25.21_PM.png Viewing one of the groups from config bar https://reviews.apache.org/media/uploaded/files/2014/05/08/6b8e6077-c8c8-48c9-94e9-732e782204b6__Screen_Shot_2014-05-08_at_4.25.36_PM.png Viewing multiple configs at the same time https://reviews.apache.org/media/uploaded/files/2014/05/08/b7b2cba1-034b-43e6-9eaa-3ba1a78f87f5__Screen_Shot_2014-05-08_at_4.25.42_PM.png screenshot of really small group https://reviews.apache.org/media/uploaded/files/2014/05/12/2d7999ac-ba1f-45b0-bf9e-99f9c7a10009__Screen_Shot_2014-05-12_at_1.27.59_PM.png Thanks, David McLaughlin
Re: Review Request 21247: Add config grouping visualisation to job page
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21247/#review42617 --- Ship it! src/main/resources/org/apache/aurora/scheduler/http/ui/groupSummary.html https://reviews.apache.org/r/21247/#comment76435 kill extra newline src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js https://reviews.apache.org/r/21247/#comment76436 s/if(/if (/ - Bill Farner On May 8, 2014, 11:45 p.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21247/ --- (Updated May 8, 2014, 11:45 p.m.) Review request for Aurora, Suman Karumuri and Bill Farner. Bugs: AURORA-378 https://issues.apache.org/jira/browse/AURORA-378 Repository: aurora Description --- Replace the button to show/hide configs with a bar when there are multiple. Tidied up the display of the configuration data. Diffs - src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 983101277ffbd1c4017b29f4c86e61315f1bcc78 src/main/resources/org/apache/aurora/scheduler/http/ui/configSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css dc515ac818a9af36522bb07a125cd92ff9230fa2 src/main/resources/org/apache/aurora/scheduler/http/ui/groupSummary.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/job.html cd78c20bee7891a6cdfd19943a6f7e8a9dce33df src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 7c80bd769b7e80bef0822846166959925b1bf7f8 src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js a79276d2486fc1122a8b084ea0614cdae759f88c Diff: https://reviews.apache.org/r/21247/diff/ Testing --- File Attachments Job page with only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/908570f2-bef2-4370-bf08-1b5c7400c3c6__Screen_Shot_2014-05-08_at_4.26.02_PM.png Showing config for only one config group https://reviews.apache.org/media/uploaded/files/2014/05/08/1424f2c3-cf66-4ce4-a595-dc3f7647d6d2__Screen_Shot_2014-05-08_at_4.26.12_PM.png Job page with multiple config groups https://reviews.apache.org/media/uploaded/files/2014/05/08/bc9c1616-3c71-4bcc-ab40-fefc56d5e19a__Screen_Shot_2014-05-08_at_4.25.21_PM.png Viewing one of the groups from config bar https://reviews.apache.org/media/uploaded/files/2014/05/08/6b8e6077-c8c8-48c9-94e9-732e782204b6__Screen_Shot_2014-05-08_at_4.25.36_PM.png Viewing multiple configs at the same time https://reviews.apache.org/media/uploaded/files/2014/05/08/b7b2cba1-034b-43e6-9eaa-3ba1a78f87f5__Screen_Shot_2014-05-08_at_4.25.42_PM.png Thanks, David McLaughlin
