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