Mailing-List: contact reviews-help@aurora.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: reviews@aurora.incubator.apache.org
Content-Type: multipart/alternative;
 boundary="===============7055219674085898533=="
MIME-Version: 1.0
Subject: Re: Review Request 26956: Use toLocalTime angular filter rather than
 duplicate logic in controller
From: "David McLaughlin" <david@dmclaughlin.com>
To: "Kevin Sweeney" <kevints@apache.org>,
 "Joshua Cohen" <jcohen@twopensource.com>
Cc: "Aurora" <reviews@aurora.incubator.apache.org>,
 "David McLaughlin" <david@dmclaughlin.com>
Date: Tue, 21 Oct 2014 00:02:57 -0000
Message-ID: <20141021000257.1283.33396@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "David McLaughlin" <noreply@reviews.apache.org>
References: <20141020231108.1283.89248@reviews.apache.org>
In-Reply-To: <20141020231108.1283.89248@reviews.apache.org>
Reply-To: "David McLaughlin" <david@dmclaughlin.com>

--===============7055219674085898533==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


> On Oct. 20, 2014, 11:11 p.m., Joshua Cohen wrote:
> > It's not entirely clear from the screenshot that "LOCAL" is a reference to the timezone. Maybe it's the fact that it's capitalized, but it feels more related to the task state than the time (I'm envisioning questions like "what does LOCAL - PENDING mean?"). Am I being crazy? Would it make sense to change it to something like "(local time)", or even better, instead of hardcoding "LOCAL" maybe just display their actual time zone identifier (or, since that's a minor hassle from JS, maybe just the offset)?
> 
> David McLaughlin wrote:
>     Hmmm, I can see your point but I found the underline/highlight on the status made it very clear they were separate entities. Is it too subtle? We also have a request to show UTC so it might be time to rethink the display of task events. 
>     
>     (FWIW I would much rather show timezone, but it is deprecated in moment.js for the usual reasons and I don't think offset is a good user experience.)
> 
> Joshua Cohen wrote:
>     Is http://momentjs.com/timezone/ what's deprecated? Or are we just wary of introducing another dependency?
> 
> David McLaughlin wrote:
>     That is a library to replace the deprecation I was talking about (you can use 'z' in the formatting string, but it hard fails in certain time zones). But yeah I'd prefer to avoid adding a new dependency to solve this, at least until we do a better job with JS dependency management.
> 
> David McLaughlin wrote:
>     Alternative proposal: wrap toLocalTime in a try catch and only show local if the moment.js call using 'z' fails?
> 
> Joshua Cohen wrote:
>     Sounds reasonable.
>     
>     Is there a ticket tracking the larger problems w/ JS depdency management?

There is sort of a catch-all of a bunch of issues here: https://issues.apache.org/jira/browse/AURORA-451


- David


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


On Oct. 20, 2014, 10:55 p.m., David McLaughlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26956/
> -----------------------------------------------------------
> 
> (Updated Oct. 20, 2014, 10:55 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Kevin Sweeney.
> 
> 
> Bugs: AURORA-873
>     https://issues.apache.org/jira/browse/AURORA-873
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Fixes the AM/PM issue by using 24 hour clock format that was already defined in filters.js. 
> 
> 
> Diffs
> -----
> 
>   src/main/resources/scheduler/assets/js/controllers.js f8e2eb7b896843bb19ad3ea5108532209603c73c 
>   src/main/resources/scheduler/assets/taskStatus.html ae32866ff241e18d04a82e8a7f909cb692657188 
> 
> Diff: https://reviews.apache.org/r/26956/diff/
> 
> 
> Testing
> -------
> 
> See attached screenshot. 
> 
> 
> File Attachments
> ----------------
> 
> screenshot
>   https://reviews.apache.org/media/uploaded/files/2014/10/20/d89be4fc-7fae-4746-841a-ccaf66e2439e__Screen_Shot_2014-10-20_at_3.55.15_PM.png
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>


--===============7055219674085898533==--