> 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 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).
> 
> David McLaughlin wrote:
>     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.

Ok. we can discuss the cleanup and code style else where. 


- Suman


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


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

Reply via email to