Skip to:
Content

Opened 4 years ago

Closed 23 months ago

Last modified 21 months ago

#5757 closed enhancement (fixed)

Add a "data-timestamp" attribute to all templates requiring timestamps

Reported by: r-a-y Owned by: r-a-y
Milestone: 2.7 Priority: normal
Severity: normal Version:
Component: Core Keywords: has-patch dev-feedback i18n-change early commit
Cc: espellcaste@…

Description

This is kind of tied to the widget caching ticket - #3659.

Before we can even think about fragment caching, there are some issues to overcome.

One of them is caching any BP loop that has a timestamp inside the loop (eg. "active X minutes ago"). If the loop is cached, then the timestamp is also cached and will not be reflective of the real timestamp.

To remedy this, I plan on introducing a "data-timestamp" attribute to our BP loop functions / templates where appropriate. Then, it would be possible with JS (perhaps using jQuery timeago) to dynamically swap out the cached timestamp with the real relative time.

Attachments (3)

6613.patch (2.9 KB) - added by imath 3 years ago.
5757.01.patch (164.1 KB) - added by r-a-y 2 years ago.
5757.02.patch (165.8 KB) - added by r-a-y 2 years ago.

Download all attachments as: .zip

Change History (37)

#1 @DJPaul
4 years ago

  • Keywords good-first-bug needs-patch added

#2 @r-a-y
4 years ago

  • Milestone changed from 2.2 to Future Release

After thinking about this some more, I think this is best left to plugins for now.

See my plugin to do this here if interested:
https://github.com/r-a-y/bp-relative-time

#3 @DJPaul
4 years ago

  • Keywords good-first-bug removed

#4 @boonebgorges
3 years ago

See #6613.

r-a-y, any new thoughts about why this is best left to plugins? With minimal mods to our markup (the attribute suggested here), we could introduce a library that would be 100% progressive enhancement, since it'd all happen in JS.

My main concern is with localization. http://momentjs.com/ seems like it supports a fairly large number of locales out of the box. Are we OK with leaving l18n support to the library in cases like this? (Seems like it's a case where we simply wouldn't load the library if it doesn't support the locale.)

#5 @DJPaul
3 years ago

It is possible to load translations into MomentJS via wp_localize_script, I've done that before. I found it preferable because the version of MomentJS that comes with the locales is all concatenated into one file (http://momentjs.com/downloads/moment-with-locales.js) ; the translations would add about 30kb to each page load where we enqueued MomentJS, regardless of what langauage is/isn't used.

#6 @r-a-y
3 years ago

My plugin is using livestamp.js, which is a jQuery plugin for Moment.js and handles localization.

At the time that I placed this in FR, it was too close to introduce new functionality that late in the release cycle.

But, we can explore this in 2.4 if we want.

#7 @Tafmakura
3 years ago

As a typical user I think this feature is critical, this is standard practice with most other social networks to have an activity feed that, well.... just works... and I wouldn't want my buddypress powered site having a wonky activity feed that makes no sense to the user. This should be an out of the box feature, users don't appreciate plugins that rely on other plugins for basic core functionality.

Is it not possible for buddypress to release a jetpack like collection of plugins that can serve as a bridge between Core features and must have features that are not ready for core, things like the followers plugin and bp relative come to mind.If there is an existing ticket please guide me to that ticket or I shall open one "forthwith"

#8 @boonebgorges
3 years ago

It is possible to load translations into MomentJS via wp_localize_script

Awesome. Bonus: I think that all the strings we'd need for our use of moment.js are already in our language pack: '%s ago', and the '%s seconds/minutes/hours/days/months/years' strings. So it'd probably work in all supported languages without additional translation.

But, we can explore this in 2.4 if we want

if there's dev time to do it, I think we should. It looks to me like the plugin is extremely simple. I'd suggest that this utility *not* be part of buddypress.js, as it's not really theme-dependent and IMO there's no reason to tie it to bp-legacy.

#9 @r-a-y
3 years ago

  • Milestone changed from Future Release to 2.4
  • Owner set to r-a-y
  • Status changed from new to assigned

Looks like there is enough demand for this. So let's do it!

#10 @espellcaste
3 years ago

  • Cc espellcaste@… added

#11 @imath
3 years ago

As #6613 was closed as a duplicate to this one...

6613.patch is a way to use the Activity Heartbeat feature to refresh the displayed time-since spans each time we get the heartbeat tick.

If we only need to update the activity directory, then it can avoid a new JS library. if we need to update more timestamps, then i guess this patch is not enough :)

@imath
3 years ago

#12 @DJPaul
3 years ago

  • Milestone changed from 2.4 to 2.5

#13 @r-a-y
2 years ago

  • Milestone changed from 2.5 to 2.6

I promise this will be addressed in 2.6!

@r-a-y
2 years ago

#14 @r-a-y
2 years ago

  • Keywords has-patch added; needs-patch removed

So I did promise a patch for 2.6, so here it is!

Whether it's too late for 2.6, I'll leave up to you :)

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


2 years ago

#16 @dcavins
2 years ago

  • Keywords dev-feedback added

This is great. The only question I have about it is that the time strings output by the JS library are not the same as we currently generate:
"1 week, 4 days ago" becomes "12 days ago"
"3 weeks, 3 days ago" becomes "25 days ago"
"3 months, 1 week ago" becomes "3 months ago"
"7 months, 4 weeks ago" becomes "8 months ago"

It appears, from the localization file, that the JS library uses the increments:
a few seconds ago (less than a minute)
## minutes (up to an hour)
## hours (up to a day)
## days (up to a month)
## months (up to a year)
## years

It suits me, but are we OK with this change? (Devs can always have the old strings back by dequeueing the js files.)

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


2 years ago

#18 @imath
2 years ago

@r-a-y wp_add_inline_script() was introduced in WordPress 4.5 i'm afraid the use of this function will do some damages for people using a previous version of WordPress

NB: if i've suggested the use of this function for the activity embed ticket that's because, the activity embeds are requiring WP 4.5

#19 @DJPaul
2 years ago

I don't have a problem with the new strings; they're a bit shorter and simpler.

http://momentjs.com/docs/#/customization/relative-time-threshold/ explains the defaults for what it considers a month, etc. We can customise these and the strings if we wanted.

#20 @r-a-y
2 years ago

NB: if i've suggested the use of this function for the activity embed ticket that's because, the activity embeds are requiring WP 4.5

Argh, good catch, imath!

I wasn't sure if we wanted to go with an external JS file or an inline one. For 02.patch, I opted for inline and added some backpat support for < WP 4.5.


The only question I have about it is that the time strings output by the JS library are not the same as we currently generate

I also encountered something similar as the localization is missing the context.

eg. active 5 seconds ago becomes 5 seconds ago.

For our widgets that use created X ago or active X ago, this becomes a little problematic.

Is this a dealbreaker?

Last edited 2 years ago by r-a-y (previous) (diff)

@r-a-y
2 years ago

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


2 years ago

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


2 years ago

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


2 years ago

#24 @dcavins
2 years ago

  • Keywords i18n-change early added
  • Milestone changed from 2.6 to 2.7

#25 @DJPaul
2 years ago

So, this was bumped because it changed the strings in the way that @r-a-y describes above, and we couldn't agree if this was good or not? Is that an accurate recollection?

#26 @dcavins
2 years ago

This was bumped because we weren't 100% confident in the localization friendliness of the new strings. I think the localization friendliness was about a neutral change when compared to the old strings, but we didn't have time to all feel good about it before beta. This should go in like this week, then we can tweak the strings as we learn more about how they fail/cause problems.

#27 @DJPaul
2 years ago

  • Keywords commit added

Good idea!

#28 @djpaul
23 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 11008:

Fix inaccurate timestamps caused by page caching.

If any template with a timestamp is statically cached, then the timestamps quickly become inaccurate.
The change adds livestamp.js and moment.js to dynamically update timestamps with the real relative time.

A consequence is that the labels surrounding the timestamps, notably in our widgets, have slightly changed.
We're going to keep an eye on this for the remainder of the 2.7 development cycle, and may make further adjustments as testing proves necessary.

Fixes #5757

Props r-a-y, imath, DJPaul

#29 @DJPaul
23 months ago

I tweaked this a little on its way in; a few function names, etc, and added in some missing optional parameters to functions that had been changed. Nothing big.

#30 @boonebgorges
22 months ago

In 11016:

Fix incorrect function name in Friends widget.

Introduced in [11008].

See #5757.

#31 @boonebgorges
22 months ago

In 11017:

Fix incorrect function name in Members widget.

Introduced in [11008].

See #5757.

#32 @dcavins
22 months ago

I started to change bp_get_group_member_joined_since() to use this method, but realized that if we modified bp_core_time_since() we could probably catch more instances in a single pass. Or are we leaving bp_core_time_since() alone for backcompat reasons?

#33 @r-a-y
21 months ago

In 11097:

Core: Fix fatal error with bp_core_get_iso8601_date() when an invalid date string is passed.

This error occurred on the Members Directory and selecting the
"Alphabetical" filter when attempting to parse an inactive user's last
activity. Inactive users passes the "Never active" string to
bp_core_get_iso8601_date() and the DateTime class causes a fatal
error on invalid date strings, unless we catch the exception, which this
commit is now doing.

See #5757.

#34 @r-a-y
21 months ago

In 11098:

bp-legacy: Add data-livestamp attribute to member header.

Missed this instance during initial implementation of livestamp.js.

See #5757.

Note: See TracTickets for help on using tickets.