#51130 closed enhancement (fixed)
Events displayed in venue timezone instead of user's
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 5.5.2 | Priority: | high |
Severity: | normal | Version: | |
Component: | Date/Time | Keywords: | has-patch has-unit-tests |
Focuses: | Cc: |
Description
#meta4480 describes how the Events API returns data as a "WP timestamp" (Unix timestamp + UTC offset), rather than a true Unix timestamp, which is always UTC.
Because of that, events are displayed in the venue's timezone, rather than the user's. That's usually accurate for in-person events, but usually inaccurate for online events, where attendees can be located in any timezone.
Once the API starts returning a proper Unix timestamp, Core should update the widget to display events in the current user's timezone.
cc @rarst, @dd32
Attachments (3)
Change History (58)
#2
@
8 months ago
Ah, ok, I couldn't find much info on them beyond just "Unix timestamp plus UTC offset". That how we convert Meetup.com API timestamps are converted to local time. What's the distinction?
#3
@
8 months ago
No, I mean your understanding of WP timestamp is correct, but at the time I opened the meta issue timestamps weren't returned, the API return was "date": "2019-06-18 19:00:00"
. I don't know if that's still the state of it, assumed so.
#4
@
8 months ago
Ah, that's right. The patch on #meta4480 will add new fields, but hasn't been committed yet.
This ticket was mentioned in PR #535 on WordPress/wordpress-develop by iandunn.
7 months ago
- Keywords has-patch has-unit-tests added
[...] events are displayed in the venue's timezone, rather than the user's. That's usually accurate for in-person events, but usually inaccurate for online events, where attendees can be located in any timezone.
See https://core.trac.wordpress.org/ticket/51130
See https://meta.trac.wordpress.org/ticket/4480
This ticket was mentioned in PR #539 on WordPress/wordpress-develop by iandunn.
7 months ago
[...] events are displayed in the venue's timezone, rather than the user's. That's usually accurate for in-person events, but usually inaccurate for online events, where attendees can be located in any timezone.
See https://core.trac.wordpress.org/ticket/51130
See https://meta.trac.wordpress.org/ticket/4480
#9
@
7 months ago
This is an excellent idea - with all meetups being online, it makes a lot more sense for the time to show in the user's timezone. Could we indicate that at the top of the widget and state which timezone it is? So for me it would say "Times shown in local timezone (UTC+2)". Your patch may already do that, but I can't test locally at the moment.
#10
follow-up:
↓ 13
@
7 months ago
That's an interesting idea 🤔
I'd thought about adding it next to the event time, but it felt too cluttered. Putting it next to the city time would mitigate that. I uploaded some screenshots above that show examples of those.
It also shows the difference between using the UTC offset, and the timezone abbreviation. I think I'm leaning towards the timezone abbreviation, since that's shorter, and would be more familiar for most people (I'm assuming).
If we go with the timezone abbreviation, then maybe it would be better to place it next to the event time? That'd be more directly connected to the time, and more likely to be seen.
There may be some technical challenges getting either of them, though, though. Date().getTimezoneOffset()
and new Date().toLocaleTimeString('en-us',{timeZoneName:'short'}).split(' ')[2]
have various issues with browser support. moment().tz("America/Los_Angeles").zoneAbbr()
might not be worth the performance hit for the enqueue. I guess we could try the two native methods, and fallback to an empty string if neither are available?
#11
@
7 months ago
xref ticket:47798#comment:31 for discussion about internationalizing the start/end month in format_event_data_time()
.
#12
@
7 months ago
As soon as they are not displayed and not passed to any hook, those strings don't need to be prepared for i18n.
#13
in reply to:
↑ 10
@
7 months ago
This sounds like a great improvement! For example user in Stockholm does not have any events coming up in Sweden, but we are having a WordCamp Finland Online later this year. Sweden is UTC+2 and we are living UTC+3 in Finland, so user currently would see wrong time if it would be meetup instead of WordCamp. And our Helsinki group might have online meetup which would interest them. Stockholm and Helsinki are so close to each other, that events in one of those cities will most likely be visible in both cities at the moment at least.
Replying to iandunn:
I'd thought about adding it next to the event time, but it felt too cluttered. Putting it next to the city time would mitigate that. I uploaded some screenshots above that show examples of those.
I think it's enough to show the timezone next to the city. Putting it next to event time makes the view too cluttered indeed. Though in that case, the user might think the event times are still displayed in the venue/local time rather than in their own timezone and add more confusion.
Some visual indication that tells how times are shown, as @hlashbrooke suggested, would be really good addition.
It also shows the difference between using the UTC offset, and the timezone abbreviation. I think I'm leaning towards the timezone abbreviation, since that's shorter, and would be more familiar for most people (I'm assuming).
Personally I'm leaning towards showing UTC offset rather than timezone abbreviation. It really depends on the location of user to which they are used to. In my mind timezone abbreviations are something that is used in US and UK, but not in central Europe for example and I've grown to use UTC offset since the elementary school :D
#14
follow-up:
↓ 15
@
7 months ago
In my mind timezone abbreviations are something that is used in US and UK, but not in central Europe for example
Ah, great point. Maybe we should use an I18n'd string to let translators pick which one, then? e.g.,
/* translators: Do not translate. Set either `abbreviation` or `offset`, in English. This determines if the Events Widget shows timezone abbreviations or offsets. Pick whichever is most common in your locale. */
if ( 'abbreviation' === __( 'events-widget-timezone-display' ) ) {
// show the timezone abbreviation
} else {
// show the UTC offset
}
#15
in reply to:
↑ 14
@
7 months ago
Sounds a bit hacky, but it works! :D If translators do not translate or translate it incorrectly, using UTC offset as a fallback +1
Actually it could be usable if core would handle the same in bit more elegant way in future - but I guess that's not going to happen in timespan which this events widget change is wanted to go out.
#16
@
7 months ago
Yeah, I've seen Core do it in other places, but I agree it'd be nice if there were a more elegant way.
I don't think there's any way to get it via the locale/tz info available via JavaScript. There may be some other canonical source, though. I'll see what I can find.
Or we could just use the UTC offset for simplicity, if that's what's most common around the world.
#17
@
7 months ago
Er, nevermind, it does look like toLocaleTimeString() will do that. #pr539 is already using it, but w/ the locale hardcoded. I wanted to remove that anyway, though. Leaving that param empty should default to the user's (browser/BCP47) locale. As long as we can still parse it out reliably, that should work.
#18
@
7 months ago
I've updated the PR to show the localized abbreviation 👍🏻
Note to self:
To manually test abbreviations and timezones on OS X, open the browser with overridden environment variables:
TZ="Europe/Helsinki" open /Applications/FirefoxDeveloperEdition.app
To verify: console.log( Intl.DateTimeFormat().resolvedOptions().timeZone );
You don't need to change the OS/browser locale, because (I'm guessing) the timezone shortname is pulled from tzdata, rather than locale data.
#20
@
7 months ago
- Milestone changed from Awaiting Review to 5.5.2
- Owner set to iandunn
- Priority changed from normal to high
- Status changed from new to assigned
#27
@
6 months ago
iandunn commented on PR #539:
I rewrote that test to stub the Date
functions, but at that point it's not testing anything that the other tests aren't. So, I just removed it.
I think this is ready for review, despite the Travis errors. I'm still digging into those, but they appear to be unrelated. Possibly related to #core51466.
#36
@
6 months ago
@iandunn Hi! I saw you moved it to milestone 5.6. While we're preparing 5.5.2, I just wanted to add that if it's a high
priority, I'm all for backporting this to 5.5.2 which will probably ship in two weeks or so :)
#38
@
6 months ago
- Milestone changed from 5.6 to 5.5.2
Ah, I didn't realize it was going to happen after all, thanks!
I'll update the @since
tags and backport 👍🏻
#42
@
6 months ago
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening for backport.
This ticket was mentioned in Slack in #core by iandunn. View the logs.
6 months ago
This ticket was mentioned in Slack in #community-team by iandunn. View the logs.
6 months ago
This ticket was mentioned in Slack in #community-events by iandunn. View the logs.
6 months ago
#48
@
6 months ago
- Resolution fixed deleted
- Status changed from closed to reopened
Huh, some of the new QUnit tests are failing in the `5.5` branch, even though they pass in trunk
.
>> dashboard > communityEvents.getFormattedDate - multiple day event should use corresponding format >> Actual: "September 15–17, 2020" >> Expected: "September 15–17, 2020" >> at Object.<anonymous> (file:///home/travis/build/WordPress/wordpress-develop/tests/qunit/wp-admin/js/dashboard.js:106:12) >> dashboard > communityEvents.getFormattedDate - multiple month event should use corresponding format >> Actual: "September 15 – October 6, 2020" >> Expected: "September 15 – October 6, 2020" >> at Object.<anonymous> (file:///home/travis/build/WordPress/wordpress-develop/tests/qunit/wp-admin/js/dashboard.js:117:12)
I saw that happen sometimes when I was writing them, but couldn't reproduce it reliably, and thought copy/pasting might have fixed it. Apparently not :)
I'm gonna take another look.
This ticket was mentioned in PR #651 on WordPress/wordpress-develop by iandunn.
6 months ago
En dashes (8211
) and hyphens (45
) are getting mixed up, causing some QUnit tests to fail. They all appear to be en dashes in the code itself, but there could be something in dateI18n
/ Moment that is converting to a hypen.
See 51130#comment:48
021e8ef demonstrates the problem, but isn't necessarily the best solution.
Trac ticket: https://core.trac.wordpress.org/ticket/51130
This ticket was mentioned in Slack in #core by iandunn. View the logs.
6 months ago
#51
@
6 months ago
Looks like some encoding failure. It would be great to find the source, but if it turns out to be too hard to consistently reproduce, it would probably be easier to replace the –
character in the affected strings with the –
entity:
%1$s %2$d–%3$d, %4$d
%1$s %2$d – %3$s %4$d, %5$d
That would make the strings less readable to translators, but should still work. I think I went with the character in [47124] for readability only.
#52
@
6 months ago
Actually, it looks like 51130.diff resolves this:
https://travis-ci.com/github/SergeyBiryukov/wordpress-develop/builds/192442752
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8" />
This line already exists in trunk since [49101], but not in the 5.5 branch. That explains the difference.
#53
@
6 months ago
SergeyBiryukov commented on PR #651:
Commented on the ticket: https://core.trac.wordpress.org/ticket/51130#comment:52
Adding this to qunit/index.html
appears to resolve the issue:
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8" />
This line already exists in trunk since [49101], but not in the 5.5 branch. That explains the difference.
Per that ticket current API return is local event date and time without time zone, not WP timestamp.