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

Reimplement relative time formatting as a filter, dropping dependency on deprecated moment.js #1450

Merged
merged 1 commit into from
Jul 24, 2021

Conversation

oldnomad
Copy link
Contributor

This PR offers a more complete solution than #1441, and replaces it.

This is a re-implementation of relative date/time formatting (getRelativeDate()), which was mentioned in #1433. Original version used NodeJS module moment.js, which is obsolete, deprecated, and slated for removal from Nextcloud base.

This version implements relative formatting as a filter, instead of a controller method. It uses pure JavaScript Intl.RelativeTimeFormat, with fallback to plain date formatting for older browsers. Dependency on moment.js is dropped. The new function is, however, slightly different from the original:

  • If the distance between now and the timestamp is more than 90 days (and for older browsers), absolute date/time format is used. I didn't want to use "N months ago" relative format, since it's difficult to calculate correctly; and "N weeks ago" seems to be less readable for long distances.
  • The function returns "N weeks ago" for distances greater than 7 days, "N days ago" for distances greater than 1 day, and so on.
  • In the fallback (absolute format) branch the function returns time only to minutes. If the timestamp is within half-day from now, only time is printed; if the timestamp is within 7 days from now, weekday is added; otherwise, full date is added.

@SMillerDev
Copy link
Contributor

LGTM

@codecov-commenter
Copy link

Codecov Report

Merging #1450 (6f0e531) into master (9087d89) will increase coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 6f0e531 differs from pull request most recent head 3fa791d. Consider uploading reports for the commit 3fa791d to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1450      +/-   ##
============================================
+ Coverage     91.93%   91.94%   +0.01%     
  Complexity      759      759              
============================================
  Files            64       64              
  Lines          2776     2769       -7     
============================================
- Hits           2552     2546       -6     
+ Misses          224      223       -1     
Impacted Files Coverage Δ
lib/Service/FeedServiceV2.php 100.00% <0.00%> (ø)
lib/Command/Debug/ItemList.php 100.00% <0.00%> (ø)
lib/Command/Debug/FeedItemList.php 100.00% <0.00%> (ø)
lib/Command/Debug/FolderItemList.php 100.00% <0.00%> (ø)
lib/Fetcher/FeedFetcher.php 89.11% <0.00%> (+0.44%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9087d89...3fa791d. Read the comment docs.

@Grotax
Copy link
Member

Grotax commented Jul 19, 2021

Could you do a rebase? We merged a fix for the tests.

… on deprecated moment.js

Signed-off-by: Alec Kojaev <alec@kojaev.name>
@oldnomad
Copy link
Contributor Author

Could you do a rebase? We merged a fix for the tests.

Done.

Copy link
Contributor

@anoymouserver anoymouserver left a comment

Choose a reason for hiding this comment

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

Works as expected.
I stumbled over the missing months though, even 12 weeks (your 90 days threshold) is not a common unit for me, making this more confusing to me than an absolute date. It's probably personal preference, but find absolute dates better to understand than large relative ones.
grafik

Regarding the improved performance, I couldn't do any tests on my test instance.

I'm not sure if this will be a longer term solution or if this will change with the new vue based front end.

@SMillerDev SMillerDev merged commit 65c15da into nextcloud:master Jul 24, 2021
@Grotax
Copy link
Member

Grotax commented Jul 24, 2021

Thank you very much @oldnomad :)

And I assume that with the vue rewrite this might be replaced, maybe until then Nextcloud also has a new lib that respects the Nextlcloud localization

Grotax added a commit that referenced this pull request Jul 24, 2021
Changed
- Added vue and ng-vue packages (#1421)
- Reimplemented relative time formatting as a filter (#1450)
- Added new `news:updater:update-user` command to update the feeds of a single user (#1360).

Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
@Grotax Grotax mentioned this pull request Jul 24, 2021
Grotax added a commit that referenced this pull request Aug 2, 2021
Changed
- Reimplemented relative time formatting as a filter (#1450)

Fixed
- Set icon offset explicitly for navigation items (#1465)

Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Grotax added a commit that referenced this pull request Aug 2, 2021
Changed
- Reimplemented relative time formatting as a filter (#1450)

Fixed
- Set icon offset explicitly for navigation items (#1465)

Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Grotax added a commit that referenced this pull request Aug 2, 2021
Changed
- Reimplemented relative time formatting as a filter (#1450)

Fixed
- Set icon offset explicitly for navigation items (#1465)

Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Grotax added a commit that referenced this pull request Aug 3, 2021
Changed
- Reimplemented relative time formatting as a filter (#1450)

Fixed
- Set icon offset explicitly for navigation items (#1465)

Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
@Grotax Grotax mentioned this pull request Mar 7, 2022
1 task
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.

None yet

5 participants