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

Add option to show browser's local time #1653

Merged
merged 8 commits into from
Jan 13, 2022

Conversation

NikitaIvanovV
Copy link
Contributor

@NikitaIvanovV NikitaIvanovV commented Jan 8, 2021

This PR adds an option to show browser's local time when viewing history, commit, latest_changes and clicking When was this page last modified? button.

I used the client-side approach: I pass time in datetime attribute of <time> tag and then JS creates a Date object, which will be in borwser's local time. To support strftime (used for formatting Time in latest_changes and history) on the client, I had to include Datejs library. I hope adding another library for such a minor thing is not a big problem?

@NikitaIvanovV
Copy link
Contributor Author

Oh no, I forgot to add tests again. Will do tomorrow!

@dometto
Copy link
Member

dometto commented Feb 13, 2021

@vichyavin this looks good to me. Perhaps you could change the documentation strings a little:

Show browser's local time instead of server's.

...is a bit unclear. If I understand the point of this PR correctly, it would perhaps be better phrased by:

`Use the browser's local timezone instead of the server's for displaying dates."

@bartkamphorst any thoughts?

@dometto
Copy link
Member

dometto commented Feb 13, 2021

@vichyavin it also looks like one of the tests is failing on JRuby:

https://travis-ci.org/github/gollum/gollum/jobs/754610273

Could you investigate? Thanks!

@NikitaIvanovV
Copy link
Contributor Author

@dometto Done!

Copy link
Member

@dometto dometto left a comment

Choose a reason for hiding this comment

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

Thanks @vichyavin. One more suggestion for refactoring in the comments. Sorry for not mentioning it earlier.

lib/gollum/views/history.rb Show resolved Hide resolved
@dometto dometto merged commit 6f87050 into gollum:master Jan 13, 2022
@dometto
Copy link
Member

dometto commented Jan 13, 2022

Again, thanks for the contribution and patience, @NikitaIvanovV!

@benjaminwil benjaminwil mentioned this pull request Feb 6, 2022
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants