> 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.
>
> David McLaughlin wrote:
> 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.
I'm in favor of giving the person behind the keyboard the majority vote here.
Sounds like David has played with both approaches and found the current diff
most user-friendly. Let's re-assess if we find otherwise.
> 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.
>
> Suman Karumuri wrote:
> Ok. we can discuss the cleanup and code style else where.
Mind having this discussion in the abstract on the dev mailing list? As a
javascript noob, I find it interesting, and wouldn't mind following along.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21247/#review42626
-----------------------------------------------------------
On May 13, 2014, 12:02 a.m., David McLaughlin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21247/
> -----------------------------------------------------------
>
> (Updated May 13, 2014, 12:02 a.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
>
>
