aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Suman Karumuri" <ma...@apache.org>
Subject Re: Review Request 21247: Add config grouping visualisation to job page
Date Mon, 12 May 2014 22:19:45 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.
> 
> 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
> 
>


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