-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19565/#review38325
-----------------------------------------------------------



src/main/java/org/apache/aurora/scheduler/http/ServletModule.java
<https://reviews.apache.org/r/19565/#comment70369>

    This method makes for difficult-to-read call-sites.  How about silent=true 
for everything to remove the extra flag?  You can then add the /scheduler link 
to / with:
    
      Registration.registerEndpoint(binder(), "/scheduler");



src/main/resources/org/apache/aurora/scheduler/http/ui/breadcrumb.html
<https://reviews.apache.org/r/19565/#comment70380>

    Consider removing the spaces on the LHS of colons:
    
      Role: myRole
      Environment: myEnv
    
    as opposed to
    
      Role : myRole
      Environment : myEnv



src/main/resources/org/apache/aurora/scheduler/http/ui/breadcrumb.html
<https://reviews.apache.org/r/19565/#comment70379>

    Is the extra "if" redundant here?



src/main/resources/org/apache/aurora/scheduler/http/ui/home.html
<https://reviews.apache.org/r/19565/#comment70381>

    columnCollection and rowCollection probably aren't the most informative 
names.  Can you be more descriptive?



src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js
<https://reviews.apache.org/r/19565/#comment70390>

    DRY



src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js
<https://reviews.apache.org/r/19565/#comment70392>

    Why not:
    
    $scope.showResources = !$scope.showResources;
    $scope.resourceButtonText = ($scope.showResources ? 'Show' : 'Hide') + ' 
Resource Consumption';



src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js
<https://reviews.apache.org/r/19565/#comment70396>

    missing semicolon



src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js
<https://reviews.apache.org/r/19565/#comment70397>

    Does it make sense to s/cronJobName/jobName/, then you can reuse the sort 
function? (This also results in more consistent naming.)
    
    You could take this a step further to declare a local function that 
produces the basic information for a job (role, environment, name), and 
decorate that with the type-specific fields.  Your call on that part.



src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js
<https://reviews.apache.org/r/19565/#comment70395>

    missing semicolon



src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js
<https://reviews.apache.org/r/19565/#comment70393>

    missing semicolon



src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js
<https://reviews.apache.org/r/19565/#comment70394>

    missing semicolon



src/main/resources/org/apache/aurora/scheduler/http/ui/role.html
<https://reviews.apache.org/r/19565/#comment70382>

    This is a copy-paste from home.html, and will probably drift.  Can you use 
a fragment instead?



src/main/resources/org/apache/aurora/scheduler/http/ui/role.html
<https://reviews.apache.org/r/19565/#comment70387>

    Right now you have a 'global' scope with:
    
      title
      error
      reloadMsg
      errorMsg
      columnCollection
      role
      environment
      quotaError
      quotaErrorMsg
      jobsTableConfig
      jobsTableColumns
      cronJobsTableConfig
      cronJobsTableColumns
      showResources
      resourceButtonText
    
    (possibly more that i've missed)
    
    And some of these have relationships.  It would be nice to have 
relationships follow the scoping (i shouldn't have an errorMsg or reloadMsg 
without an error).  Laying out a scoping convention early will also help define 
the pattern before the scope becomes unwieldy.



src/main/resources/org/apache/aurora/scheduler/http/ui/role.html
<https://reviews.apache.org/r/19565/#comment70388>

    funky indent?


- Bill Farner


On March 23, 2014, 4:35 a.m., Suman Karumuri wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19565/
> -----------------------------------------------------------
> 
> (Updated March 23, 2014, 4:35 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-39
>     https://issues.apache.org/jira/browse/AURORA-39
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Implemented the scheduler role and role/env pages using AngularJS.
> 
> Added angular route and underscore.js modules.
> 
> 
> Diffs
> -----
> 
>   3rdparty/javascript/bower_components/angular-route/.bower.json PRE-CREATION 
>   3rdparty/javascript/bower_components/angular-route/README.md PRE-CREATION 
>   3rdparty/javascript/bower_components/angular-route/angular-route.js 
> PRE-CREATION 
>   3rdparty/javascript/bower_components/angular-route/angular-route.min.js 
> PRE-CREATION 
>   3rdparty/javascript/bower_components/angular-route/angular-route.min.js.map 
> PRE-CREATION 
>   3rdparty/javascript/bower_components/angular-route/bower.json PRE-CREATION 
>   3rdparty/javascript/bower_components/underscore/.bower.json PRE-CREATION 
>   3rdparty/javascript/bower_components/underscore/.editorconfig PRE-CREATION 
>   3rdparty/javascript/bower_components/underscore/.gitignore PRE-CREATION 
>   3rdparty/javascript/bower_components/underscore/LICENSE PRE-CREATION 
>   3rdparty/javascript/bower_components/underscore/README.md PRE-CREATION 
>   3rdparty/javascript/bower_components/underscore/bower.json PRE-CREATION 
>   3rdparty/javascript/bower_components/underscore/component.json PRE-CREATION 
>   3rdparty/javascript/bower_components/underscore/package.json PRE-CREATION 
>   3rdparty/javascript/bower_components/underscore/underscore.js PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/http/DisplayUtils.java 
> 19df7889f15b4cf44e386d8ce0626cc94fdcdfba 
>   src/main/java/org/apache/aurora/scheduler/http/SchedulerzRole.java 
> e2f9ed0ea846c570de11b7dd85bc90aee6bc3342 
>   src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 
> e3ff2571d95effcf72b2047cc5840d56143a180c 
>   src/main/resources/org/apache/aurora/scheduler/http/schedulerzrole.st 
> b53f3524be052dcd5882c3e79e95e2f90aa071b8 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/breadcrumb.html 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/home.html 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/index.html 
> 36225d1e5147e30ba2cb4ddda96dec9f0f2f1dce 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/jobLink.html 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/js/app.js 
> db6ea99aeb749fd8674613e3620dc3012872e13c 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 
> 7cd534479dd2f17ffd46248ce9af1f8fe89beb97 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js 
> d2b2017a0efc70d425fd6c89ad6caaf46cb8ded5 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js 
> 81cd12c4fea473192cd7e6b6dba245e4dde30b3d 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/role.html 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/roleEnvLink.html 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/roleLink.html 
> fc25526389749e95efa87c719062dcea88935383 
> 
> Diff: https://reviews.apache.org/r/19565/diff/
> 
> 
> Testing
> -------
> 
> ./gradlew clean build run on local laptop.
> 
> 
> Thanks,
> 
> Suman Karumuri
> 
>

Reply via email to