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