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 26956: Use toLocalTime angular filter rather than duplicate logic in controller
Date Tue, 21 Oct 2014 00:16:26 GMT


> 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?
> 
> David McLaughlin wrote:
>     There is sort of a catch-all of a bunch of issues here: https://issues.apache.org/jira/browse/AURORA-451
> 
> Kevin Sweeney wrote:
>     What's the problem with JS dependency management? Does [this process](https://github.com/apache/incubator-aurora/blob/master/docs/developing-aurora-scheduler.md#developing-aurora-ui)
not work?
> 
> David McLaughlin wrote:
>     Yes it works, but it means tracking a bunch of 3rdparty source code in our repo.
These should be pulled in and installed (and unified/mindified into a single payload) as part
of a build process.
> 
> Joshua Cohen wrote:
>     That ticket captures the work to get a test framework set up, which we certainly
need, but I'm not clear on how it relates to adding new dependencies? It might change how
dependencies are managed, but it might not, and given that that work hasn't been prioritized
for ~5 months, I'm not sure it should be a blocker to introducing new JS dependencies.

As I said, it's a catch-all. If you look at the attached reviewboard, you'll see it installs
node and then uses bower to install dependencies at build time. I don't know if there is a
separate ticket for installing JS dependencies at build time.


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


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