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

Reply via email to