full published\created dates in title attr. for comments and articles #1656

acflint · 2019-01-27T20:15:02Z

What type of PR is this? (check all applicable)

Refactor
Feature
Bug Fix
Documentation Update

Description

Adding title attributes to Comment created_at and Article published_at elements displaying the full time\date.

It's currently displaying in UTC, and I don't see in the code base how you deal with time zones to ensure times\dates are displayed in the user's correct zone. That would be a nice next step for this feature.

Related Tickets & Documents

#406

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

When you hover over the published date: https://www.dropbox.com/s/0fyqvf7up63a2u5/Screenshot%202019-01-27%2015.11.39.png?dl=0
When you hover over the created date: https://www.dropbox.com/s/aeryrw8hvu0knz9/Screenshot%202019-01-27%2015.11.48.png?dl=0

Added to documentation?

docs.dev.to
readme
no documentation needed

[optional] What gif best describes this PR or how it makes you feel?

It's my first pull request for Dev, so...

CLAassistant · 2019-01-27T20:15:10Z

All committers have signed the CLA.

rhymes · 2019-01-27T20:47:03Z

Hi, congrats for your first PR, well done!

It's currently displaying in UTC, and I don't see in the code base how you deal with time zones to ensure times\dates are displayed in the user's correct zone. That would be a nice next step for this feature.

I'm not completely sure but I don't think you can do it at present, because DEV does not store anywhere the user's timezone (hence you can't change the time zone of the date during server side rendering).

Another way to do that would be to use JavaScript that's able to read the browser's timezone and change the time from there.

Both things seems outside the scope of the issue and this PR though :)

acflint · 2019-01-27T20:49:38Z

Thanks @rhymes! That's what I figured. I couldn't see any signs of the usual timezone handling code, so I assumed it wasn't being done. Should I add "UTC" to the title value, so the viewer at least knows that the time they're seeing is in UTC?

rhymes · 2019-01-27T20:59:56Z

Thanks @rhymes! That's what I figured. I couldn't see any signs of the usual timezone handling code, so I assumed it wasn't being done. Should I add "UTC" to the title value, so the viewer at least knows that the time they're seeing is in UTC?

That's a good idea! Otherwise I might assume the time is in my timezone instead of UTC

Link2Twenty · 2019-01-28T09:35:27Z

The build is currently failing with this error

1) Using the editor Submitting an article fill out form and submit
     Failure/Error: relevant_date.strftime("%e %B %Y at %I:%M%p %Z")
     
     ActionView::Template::Error:
       undefined method `strftime' for nil:NilClass

rhymes · 2019-01-28T09:47:28Z

app/models/article.rb

@@ -342,6 +342,11 @@ def tag_keywords_for_search
    tags.pluck(:keywords_for_search).join
  end

+  def long_publish_time
+    relevant_date = crossposted_at.present? ? crossposted_at : published_at
+    relevant_date.strftime("%e %B %Y at %I:%M%p %Z")


relevant_date can be nil

Thanks, resolved by adding safe navigation: relevant_date&.strftime("%e %B %Y at %I:%M%p %Z")

rhymes · 2019-01-28T09:47:58Z

app/models/comment.rb

@@ -166,6 +166,10 @@ def video
    nil
  end

+  def long_publish_time
+    created_at.strftime("%e %B %Y at %I:%M%p %Z")


created_at can be nil if the comment has not been saved to the DB yet

Thanks, resolved by adding safe navigation: created_at&.strftime("%e %B %Y at %I:%M%p %Z")

rhymes · 2019-01-28T09:51:29Z

app/models/article.rb

@@ -342,6 +342,11 @@ def tag_keywords_for_search
    tags.pluck(:keywords_for_search).join
  end

+  def long_publish_time


I would use a past tense here. The normal times are all crossposted_at, published_at, updated_at and so on.

So maybe something like full_published_at or long_published_at, something like that.

I also wonder if this and the other method in Comment shouldn't be in their respective decorators: article_decorator.rb and comment_decorator.rb -

I say this because it feels like just presentation logic, not really part of the business logic.

I was following the example set by readable_publish_date, which lives in both the Comment and Article models, but I agree a decorator makes more sense for both of them. I don't think moving the existing readable_publish_date methods should be part of this PR, but I will move my new methods into decorators. There don't seem to be any specs for decorators, should there be?

Hmmm I know where @rhymes is going with this but I feel like the ..._at protocol like created_at, updated_at should possibly be reserved for values which are a raw timestamp where any time can be calculated from.

Where this might be better the case for a different naming convention. I'd guess that was the original logic at least. However, that isn't to say the current convention which @acflint was following is the correct one.

Yeah, decorator makes sense. We don't have current specs there. We definitely could. Decorator code is typically decently well-covered by request specs, but unit tests couldn't hurt!

app/decorators/article_decorator.rb

Co-Authored-By: acflint <acflint@users.noreply.github.com>

abraham · 2019-01-29T21:16:44Z

This would be a good use of I18n's Date/Time format feature. You could implement that with the following:

In config/locales/en.yml add

en:
  time:
    formats:
      long: "%e %B, %Y at %I:%M%p %Z"

Remove long_published_at from CommentDecorator and ArticleDecorator.

Add this to ArticleDecorator:

def published_at
  crossposted_at.presence || super
end

Then Articles and Comments could be rendered like this:

<%= l(article.decorate.published_at, format: :long) %>
<%= l(comment.published_at, format: :long) %>

maestromac · 2019-03-15T16:20:43Z

Thank you for this PR @acflint ! Sorry we didn't get to this sooner. @rhymes took your PR as an inspiration and made it compatible with user's local time (#1970). If you could resolve the merge conflict, i'm happy to accept your share of work for the article's portion. Or i'm happy take over this PR if you'd like.

maestromac · 2019-05-09T15:23:59Z

I'm sorry to have to close your first PR on DEV 😔, but it's been sitting around for bit now.

Superseded by #2722

full published\created dates in title attr. for comments and articles

7d1393e

pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Jan 27, 2019

added timezone to time value. moved value to model. wrote specs.

a564350

rhymes reviewed Jan 28, 2019

View reviewed changes

weekly-digest bot mentioned this pull request Jan 28, 2019

Weekly Digest (21 January, 2019 - 28 January, 2019) #1668

Closed

renamed and moved long_published_at into decorators.

95c94ea

benhalpern requested review from maestromac and jessleenyc January 28, 2019 15:11

abraham reviewed Jan 29, 2019

View reviewed changes

app/decorators/article_decorator.rb Outdated Show resolved Hide resolved

Update app/decorators/article_decorator.rb

77af455

Co-Authored-By: acflint <acflint@users.noreply.github.com>

weekly-digest bot mentioned this pull request Feb 4, 2019

Weekly Digest (28 January, 2019 - 4 February, 2019) #1730

Closed

weekly-digest bot mentioned this pull request Feb 18, 2019

Weekly Digest (11 February, 2019 - 18 February, 2019) #1816

Closed

rhymes mentioned this pull request Mar 1, 2019

Datetime for thread #406

Closed

This was referenced Mar 4, 2019

Weekly Digest (25 February, 2019 - 4 March, 2019) #1955

Closed

Weekly Digest (4 March, 2019 - 11 March, 2019) #2020

Closed

This was referenced Apr 29, 2019

Weekly Digest (22 April, 2019 - 29 April, 2019) #2598

Closed

Weekly Digest (29 April, 2019 - 6 May, 2019) #2703

Closed

weekly-digest bot mentioned this pull request May 6, 2019

Weekly Digest (29 April, 2019 - 6 May, 2019) #2718

Closed

rhymes mentioned this pull request May 9, 2019

Show localized date/time on hover for articles/comments #2722

Merged

7 tasks

maestromac closed this May 9, 2019

full published\created dates in title attr. for comments and articles #1656

full published\created dates in title attr. for comments and articles #1656

acflint commented Jan 27, 2019

CLAassistant commented Jan 27, 2019 •

edited

rhymes commented Jan 27, 2019

acflint commented Jan 27, 2019

rhymes commented Jan 27, 2019

Link2Twenty commented Jan 28, 2019

rhymes Jan 28, 2019 •

edited

acflint Jan 28, 2019

rhymes Jan 28, 2019 •

edited

acflint Jan 28, 2019

rhymes Jan 28, 2019 •

edited

acflint Jan 28, 2019

benhalpern Jan 29, 2019

abraham commented Jan 29, 2019

maestromac commented Mar 15, 2019

maestromac commented May 9, 2019

full published\created dates in title attr. for comments and articles #1656

full published\created dates in title attr. for comments and articles #1656

Conversation

acflint commented Jan 27, 2019

What type of PR is this? (check all applicable)

Description

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Added to documentation?

[optional] What gif best describes this PR or how it makes you feel?

CLAassistant commented Jan 27, 2019 • edited

rhymes commented Jan 27, 2019

acflint commented Jan 27, 2019

rhymes commented Jan 27, 2019

Link2Twenty commented Jan 28, 2019

rhymes Jan 28, 2019 • edited

Choose a reason for hiding this comment

acflint Jan 28, 2019

Choose a reason for hiding this comment

rhymes Jan 28, 2019 • edited

Choose a reason for hiding this comment

acflint Jan 28, 2019

Choose a reason for hiding this comment

rhymes Jan 28, 2019 • edited

Choose a reason for hiding this comment

acflint Jan 28, 2019

Choose a reason for hiding this comment

benhalpern Jan 29, 2019

Choose a reason for hiding this comment

abraham commented Jan 29, 2019

maestromac commented Mar 15, 2019

maestromac commented May 9, 2019

CLAassistant commented Jan 27, 2019 •

edited

rhymes Jan 28, 2019 •

edited

rhymes Jan 28, 2019 •

edited

rhymes Jan 28, 2019 •

edited