> 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.
> 
> Suman Karumuri wrote:
>     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.

Well, there is no need for a visualisation when everything is homogenous. You 
just need one link to show or hide the config, similar to what we have right 
now. The purpose of the visualisation is to grab the user's attention and let 
them know - hey this job has special instances that you may not know about 
(failed updates, canaries, etc.). Always showing the bar is sort of like an 
'everything is ok alarm'.

The auto-refresh use case - in practice it would only appear when the update is 
in progress and disappear when it is complete. If it is jarring then it is so 
by design ("hey! look at this! something interesting is happening!"). It will 
not flicker during the update itself. 

I'll let someone else tie-break this though. Remember that this is just an 
initial design, when we add more features to this page and receive user 
feedback, we will adapt to that. 


> 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/#fcomment10>
> >
> >     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.
> 
> Suman Karumuri wrote:
>     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..)?

I think screenshots are how we demonstrate UI features right now. If you run 
the local UI in development mode usually you can find one job which has 
multiple groups. 

Right now I add this manually underneath $scope.taskSummary = ... on line 444:

      var dummyData =   {
        "range": {
          "start": 5,
          "end": 23
        },
        "schedulingDetail": {
          "numCpus": 1,
          "ramMb": 1024,
          "diskMb": 1024,
          "isService": false,
          "production": false,
          "contact": "",
          "ports": "",
          "constraints": "host:{\"limit\":1}, rack:{\"limit\":1}",
          "metadata": "package:14, role:mesos"
        }
      };

      if(window.location.hash) {
        $scope.taskSummary.push(dummyData);
      }


We should have a separate ticket for adding testing to the AngularJS app. I was 
surprised there were no existing tests for me to add to.


> 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.
> 
> Suman Karumuri wrote:
>     Good idea. We should move to using tooltips.

For posterity: for this case, I managed to avoid both by switching the 
container div to an a tag and adding a title. The nice part of this solution is 
now the whole bar triggers the mouseover.


> 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.
> 
> Suman Karumuri wrote:
>     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.

If JobController should not know how summary data is presented, then why should 
a separate controller know? Also, why weren't separate controllers created in 
the job controller for other similar components?

The existing task summary code is still used. The code I've added would have to 
be done as a second pass through the data as it depends on the total number of 
tasks.


> 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.
> 
> Suman Karumuri wrote:
>     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.

So you're saying to understand all the variables a function uses, you'd prefer 
to have to look outside the function body rather than have all variable 
initialization inside it? Literally the only reason to have these functions 
inside this controller is to group code together logically so it's easier to 
read and reason about. This makes it only easier to do that. So I'm going to 
keep all this code together the way it is.

Also, JavaScript has function scope. I strongly disagree that initializing 
variables outside of a function has ever been considered  best practice.


- David


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