> On Oct. 20, 2014, 4: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.
> 
> David McLaughlin wrote:
>     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.

While the JS dependency management story is suboptimal, I don't think we should 
avoid using libraries because of it.


- Kevin


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


On Oct. 20, 2014, 3: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, 3: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
> 
>

Reply via email to