Skip to content

Conversation

rhymes
Copy link
Contributor

@rhymes rhymes commented Mar 4, 2019

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

  • Refactor
  • Feature
  • Bug Fix
  • Documentation Update

Description

This PR does two main things:

  • embeds a ISO 8601 timestamp in each comment-date tag
  • displays such timestamp as a title (hence triggered by hover) with a string converted to the the user/browser's locale

It contains a few additional things:

  • I've moved .eslintrc.js from the main folder to app/assets/javascripts and set the correct ECMA version. The reason is that my VS Code kept complaining the code wasn't ES2016, which it isn't, it's ECMA 5. The folder with the "modern" JavaScript is app/javascript and it already has its own ESLint configuration file.

  • I've deleted app/assets/javascripts/utilities/getLocalTime.js because it wasn't used anywhere and it contained a duplicate function already contained inside initializeTimeFixer.js

A note: I couldn't test it with the "reading list -> comment activity" comments because I'm swamped by async jobs that are taking forever. If anybody has suggestions about what to do with those (beside delete them) please let me know. This is my current situation locally:

screenshot_2019-03-04 delayed job web

Related Tickets & Documents

Closes #406

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

This is how it looks if I set my browser to Italian for example:

screenshot 2019-03-04 at 5 58 43 pm

Added to documentation?

  • docs.dev.to
  • readme
  • no documentation needed

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

i am a master of time

@lightalloy
Copy link
Contributor

@rhymes As for the async jobs: maybe you haven't run them for a long time, and the jobs just kept adding? (just for the clarification)
In some cases, an async job adds several more jobs, and overall they take a long time to run. I optimized some of these cases, but probably there're more of these.

@lightalloy
Copy link
Contributor

Thanks for providing a separate eslint config, I struggled with its rules in the old code a bit )

@rhymes
Copy link
Contributor Author

rhymes commented Mar 5, 2019

@lightalloy it's totally possible :D I "fixed it" by deleting all the jobs from PostgreSQL, fresh start.

Thanks for providing a separate eslint config, I struggled with its rules in the old code a bit )

Yeah me too, I wonder if there's a "plan" to slowly migrate those files to either Preact or ES6.

I'll take a look at the failed tests :)

@lightalloy
Copy link
Contributor

I think deleting is a good enough solution in your case )

I haven't heard of any plan for migration to es6 yet 👀, but that would be cool

@rhymes rhymes changed the title [WIP] Display comment published timestamp according to user's locale Display comment published timestamp according to user's locale Mar 5, 2019
@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Mar 5, 2019
Copy link
Contributor

@maestromac maestromac left a comment

Choose a reason for hiding this comment

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

LGTM! Awesome work once again @rhymes !

@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Mar 8, 2019
@maestromac maestromac merged commit 3c6a7d4 into forem:master Mar 8, 2019
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Mar 8, 2019
@rhymes rhymes deleted the rhymes/add-time-to-comments branch March 8, 2019 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants