Skip to content
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

[webui] show timestamps in package revisions #2469

Merged

Conversation

DavidKang
Copy link
Contributor

Related with issue #2164

Before:
screenshot before

After:
screenshot after

@DavidKang
Copy link
Contributor Author

@@ -21,7 +21,9 @@
committed
<% end %>
<% end %>
<%= fuzzy_time(Time.at(commit['time'].to_i)) %> (revision <%= commit['rev'] %>)
<% time = Time.at(commit['time'].to_i) %>
<span title="<%= time.utc %>" class='commit-fuzzy-time'><%= fuzzy_time(time) %></span>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about integrating this into the fuzzy_time helper instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But fuzzy_time is called from different places, do we want to change this everywhere?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I don't like rendering UTC. And I think that the seconds are not necessary.

What about something like this:

time.strftime("%Y-%m-%d %H:%M")

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing it everywhere, where it makes sense, sounds like a good idea. So that we stay consistent.

@bgeuken
Copy link
Member

bgeuken commented Dec 15, 2016

Nice one

@hennevogel hennevogel added the Frontend Things related to the OBS RoR app label Dec 15, 2016
Ana06
Ana06 previously requested changes Dec 15, 2016
@@ -21,7 +21,9 @@
committed
<% end %>
<% end %>
<%= fuzzy_time(Time.at(commit['time'].to_i)) %> (revision <%= commit['rev'] %>)
<% time = Time.at(commit['time'].to_i) %>
<span title="<%= time.utc %>" class='commit-fuzzy-time'><%= fuzzy_time(time) %></span>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I don't like rendering UTC. And I think that the seconds are not necessary.

What about something like this:

time.strftime("%Y-%m-%d %H:%M")

@coolo
Copy link
Member

coolo commented Dec 15, 2016

As long as the world hasn't agreed on a single timezone, you have to specify which one was used on the server. That's actually the charme of solutions like jquery.timeago where the date is rendered by the browser.

Copy link
Member

@Ana06 Ana06 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coolo

As long as the world hasn't agreed on a single timezone, you have to specify which one was used on the server. That's actually the charme of solutions like jquery.timeago where the date is rendered by the browser.

I would prefer using jquery than rendering UTC

human_time_ago = time_ago_in_words(time) + ' ago'

if with_fulltime
raw("<span title='#{time.utc}' class='fuzzy-time'>#{human_time_ago}</span>")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer not to render seconds.

@hennevogel
Copy link
Member

Using jquery.timeago would only make sense if all times are displayed in the local client timezone. They are not, so this would be very confusing I guess.

@DavidKang DavidKang force-pushed the show_timestamps_on_package_revisions branch from 157fae0 to 34d4d50 Compare December 16, 2016 15:21
@DavidKang
Copy link
Contributor Author

@Ana06, when you can, please check if the changes.

it { expect(fuzzy_time(Time.now)).to eq('now') }
end

context 'with_fulltime' do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this testing? :-) strftime?

@hennevogel hennevogel dismissed Ana06’s stale review December 19, 2016 10:02

All feedback has been addresses

@hennevogel hennevogel merged commit 52b5f1f into openSUSE:master Dec 19, 2016
@DavidKang DavidKang deleted the show_timestamps_on_package_revisions branch December 21, 2016 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Frontend Things related to the OBS RoR app
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants