aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David McLaughlin" <da...@dmclaughlin.com>
Subject Re: Review Request 21247: Add config grouping visualisation to job page
Date Sat, 10 May 2014 08:46:36 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message