Add latest Diff timestamp. Review last activity does not represent diff activity.

Review Request #4898 — Created Nov. 5, 2013 and submitted

Valdis
Review Board
master
reviewboard

Review last activity does not represent diff activity. One can not see if the newer Diff has been uploaded or just comment added.

Tested manually on my development RB install.


Description From Last Updated

Okay, one more change I noticed. Can you wrap this in {% blocktrans %} like the blocks above it?

daviddavid

Why is this using a custom timestamp format when the others aren't? It should be consistent.

chipx86chipx86

"Diff" shouldn't be capitalized. I'm also not sure we should be outputting both the absolute and relative timestamps here, since ...

chipx86chipx86
VA
VA
  1. Ship It!
  2. 
      
david
  1. Vlad,

    I'm sorry for the delay. This patch doesn't apply cleanly to master anymore. Would you mind rebasing it and updating this review request?

    1. Hi David,

      I have uploaded newer diff (r3). I hope I made rebase right.

  2. 
      
VA
VA
david
  1. 
      
  2. Okay, one more change I noticed. Can you wrap this in {% blocktrans %} like the blocks above it?

    1. ok, done. New diff uploaded.

  3. 
      
VA
chipx86
  1. Can you provide a screenshot of how this looks? Preferably on a 2.0 beta build, as we're unlikely to change any layout of the review request page for 1.7.x.

    1. OK, the screenshot for 2.0 b2 uploaded.

  2. Why is this using a custom timestamp format when the others aren't? It should be consistent.

    1. H:MM p.m. time format is not really world wide. Though for the consistency I changed "N j, Y, H:i T" to "F jS, Y, P T" which is more common in the RB code.

    2. It shouldn't specify a format string at all. If you need customization, there's a Django settings.* variable for it.

    3. OK, the format string and "date" filter completely removed. The diff updated. I found other "F jS, Y, P T" strings by grepping RB source code.

  3. "Diff" shouldn't be capitalized.

    I'm also not sure we should be outputting both the absolute and relative timestamps here, since we don't elsewhere.

    1. ok, "Diff" has beeb lowercased. Yes, I had the same dillemma. Well, it's up to you to decide, I am only insisting on absolute timestamp which is precise even days after.

    2. Mousing over a relative timestamp shows the absolute one.

    3. wow.. This feature works like an easter egg. I would not say it is obvious or handy.

    4. OK, I am ready to remove the absolute timestamp for consistency and leave only relative one.

    5. Sounds good. Can you post a new revision of the patch?

  4. 
      
VA
VA
VA
VA
david
  1. Ship It!
  2. 
      
VA
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (aff26cd). Thanks!
Loading...