-
Notifications
You must be signed in to change notification settings - Fork 444
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adjust time format in maintenance listing table to include year #364
Adjust time format in maintenance listing table to include year #364
Conversation
Hey @samudary, thanks for the PR! Agree it be better to make it less ambiguous. What do you think of showing the relative time in the table (like "5 months ago"), with full time in a |
@ankane I like that! Pushed another commit with that change. Example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, looks great. Left two minor comments inline.
app/helpers/pg_hero/home_helper.rb
Outdated
@@ -26,5 +26,15 @@ def pghero_remove_index(query) | |||
ret << ", column: #{columns.inspect}" if columns | |||
ret | |||
end | |||
|
|||
def formatted_vacuum_times(time) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's prefix helpers with pghero_
since I think these will be global.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call! Updated
app/helpers/pg_hero/home_helper.rb
Outdated
end | ||
|
||
def formatted_date_time(time) | ||
l time.in_time_zone(@time_zone), format: "%d %b %Y %H:%M" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think we can probably use :long
since it no longer needs to fit into the table cell.
Thanks @samudary! |
Fixes #363
Rails only includes the following time format options: https://github.com/svenfuchs/rails-i18n/blob/master/rails/locale/en-US.yml#L211-L214
To make the timestamp displayed for the last vacuum or analyze less ambiguous I decided to add a year. I provided a custom format string to maintain the existing "day short_month ... 24-hour clock time" format instead of using
:long
which would completely change the order.Example: