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

Introduce time helper module with relative time method #98

Closed
wants to merge 3 commits into
base: develop
from

Conversation

Projects
None yet
6 participants
@davydovanton
Copy link
Member

davydovanton commented Feb 28, 2017

So, I think time helpers will helpful for other developers too.

This changes for 1.1.0 release

/cc @hanami/core @jodosha @cllns

@davydovanton davydovanton self-assigned this Feb 28, 2017

@davydovanton davydovanton requested a review from jodosha Feb 28, 2017

@TiteiKo

This comment has been minimized.

Copy link
Contributor

TiteiKo commented Feb 28, 2017

I'm sorry but I don't think it's a good idea... (and I'm against hard coded english translations)

Displaying time formatted this way server side is generally a bad idea as it removes the possibility to cache the page. Furthermore, the text won't update itself as the user stays on the page. There's a few js libraries that are doing exactly this.

@cllns
Copy link
Member

cllns left a comment

I could go either way on this. I'm generally in favor of adding things that'll help developers, but this might be out of scope. As @TiteiKo said, this isn't a best practice and one of Hanami's key advantages (for me) is that it encourages good practices :)
https://github.com/basecamp/local_time seems like an good solution, though I haven't used it

describe '#in_words' do
describe 'with from date' do
let(:date_from) { Time.now + 1 * hour }
it { @view.created_at(date, date_from).must_equal 'a hour ago' }

This comment has been minimized.

@cllns

cllns Feb 28, 2017

Member

should be an hour (since "hour" sounds like "our", it's treated as though it begins with a vowel... silly english 🙃)

@AlfonsoUceda

This comment has been minimized.

Copy link
Member

AlfonsoUceda commented Feb 28, 2017

Hi folks, @davydovanton thanks for this PR, I think it's a good idea but let me explain. @TiteiKo, @cllns you are right, there are JS libraries for updating the time in real time but I think it's good provide this helper, why? because when you render the time and the JS load, there may be a gap that you can see a glitch between the server rendering and the client rendering.

@davydovanton why not use I18n for this? people could have an use case for using another language.

@davydovanton

This comment has been minimized.

Copy link
Member

davydovanton commented Feb 28, 2017

@AlfonsoUceda thanks for the feedback 👍
It's a simple version of helper. I think we need to use small iterations and we can add i18n in future (I don't like long PRs). That's why I (or other developers) can improve (or add new helpers) in future. I mean that this PR is just a push for next improvements

@mereghost mereghost added the discussion label Mar 7, 2017

@mereghost mereghost changed the base branch from master to develop Jun 2, 2017

@mereghost

This comment has been minimized.

Copy link
Member

mereghost commented Jun 2, 2017

@davydovanton While I can see the benefit of making it an interactive development thing, we are past 1.0, so I think shipping fully functional stuff may be a bit better than something that will probably add more tickets on the short run.

Also time math is complicated specially since daylight saving times are a thing. Not sure if we need to tackle this or just recommend some other gem to deal with it.

@jodosha

This comment has been minimized.

Copy link
Member

jodosha commented Jul 31, 2017

Given the team has expressed an opinion against this feature, I'm gonna close this.

However, we need to explore client side libraries, in order to give an answer to the developers that need this feature.

@jodosha jodosha closed this Jul 31, 2017

@jodosha jodosha deleted the time-helpers branch Jul 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment