Add local time to hover cards based on user location #1249

jorgegonzalez · 2018-04-06T03:32:16Z

Closes #1233.

Still need to add some padding as well as a clock icon like in the linked issue, any suggestions for that?

One of the issues with this solution is that nothing will render if too many API requests are made to get the user's location and timezone info, so I just made it silently fail. Any input is appreciated.

fregante · 2018-04-06T03:47:29Z

Oof, big PR :) Thanks!

I have 2 concerns though:

moment-timezone weighs at least double our whole extension:

❯ package-size moment-timezone

  package                 size    minified  gzipped
  moment-timezone@0.5.14  840 kB  414 kB    91.2 kB

Two sequential Google API requests for every timezone

fregante · 2018-04-06T03:57:18Z

I wonder if modern Intl.DateTimeFormat browser APIs can let us skip moment-timezone

fregante · 2018-04-06T04:04:28Z

It looks pretty good though; we probably can't get anything better than Google's APIs. Perhaps we can just cache the locations and avoid the current user's

fregante · 2018-04-06T04:05:44Z

This is what I see, probably the +7 can be dropped, we already have the location name

jorgegonzalez · 2018-04-06T04:19:17Z

Yeah, I'll find a better solution than moment-timezone. At least this is a nice proof of concept! I'll also implement a cache for the requests soon.

Would GMT+7 be better in those cases or should we not show the timezone at all?

jorgegonzalez · 2018-04-06T04:36:13Z

Added the clock.

fregante · 2018-04-06T07:31:58Z

We can skip moment-timezone entirely by just reading the rawOffset and dstOffset from the timezone API call:

❯ curl 'https://maps.googleapis.com/maps/api/timezone/json?location=40.741895,-73.989308&timestamp=1331161200&sensor=false'
{
   "dstOffset" : 0,
   "rawOffset" : -18000,
   "status" : "OK",
   "timeZoneId" : "America/New_York",
   "timeZoneName" : "Eastern Standard Time"
}

The offset is in seconds and can be combined with new Date().getTimezoneOffset() (it always shows the offset of the system time), which is in minutes, in the opposite direction.

Once we have the correct Date it can be formatted via Intl.DateTimeFormat.

Side note: Those APIs give a crazy amount of info. Gotta love google for this stuff. I wish they included the simple timezone info in a single call though.

https://maps.googleapis.com/maps/api/geocode/json?address=40.741895,-73.989308

fregante · 2018-04-06T07:38:33Z

source/features/add-timezone-to-hovercard.js

+		try {
+			const location = getLocationFromHovercard(
+				Array.from(hoverCardMutation.addedNodes)
+			);


The past 10 lines can probably be replaced with a select('.Popover .octicon-location') + .nextSibling.textContent

fregante · 2018-04-06T07:38:58Z

source/features/add-timezone-to-hovercard.js

+		return timezoneResponse;
+	} catch (error) {
+		return null;
+	}


No need to catch here, let it bubble up

fregante · 2018-04-06T07:41:00Z

source/features/add-timezone-to-hovercard.js

+
+function createTimezoneRequestURL(lat, lng) {
+	const timezoneAPI = 'https://maps.googleapis.com/maps/api/timezone/json?location=';
+	return `${timezoneAPI}${lat},${lng}&timestamp=1331161200&sensor=false`;


I think the timestamp should be Date.now() / 1000

Maybe it's just me, but this request seems to fail more often with Date.now() / 1000 🤔

fregante · 2018-04-06T07:42:01Z

source/features/add-timezone-to-hovercard.js

+			/>
+		</svg>
+	);
+}


This should be moved to icons.js.

As per #1099:

The extra classes should be removed. They can probably be added to a parent or via .classList.add()

version and viewbox also can be removed

fregante · 2018-04-06T07:44:15Z

source/features/add-timezone-to-hovercard.js

+				const time = moment.tz(moment(), timeZoneId).format('HH:mm');
+				const abbr = moment.tz(timeZoneId).zoneAbbr();
+				hoverCard.append(clockIcon());
+				hoverCard.append(domify(`${time} ${abbr}`));


append accepts plain strings and multiple elements, so it can be:

hoverCard.append(clockIcon(), `${time} ${abbr}`);

fregante · 2018-04-06T07:45:00Z

source/features/add-timezone-to-hovercard.js

+			}
+		} catch (e) {
+			// Silently fail when there are too many API requests
+		}


I don't think we need suppress the errors. If it's not working we should probably show why.

It will fail when either the timezoneAPI or the locationAPIrequest fails due to too many requests, but the requests often succeed on the next mouseover. How do you want to handle this?

I'd let it go to the console for now. Perhaps it can use onetime to show the error in the console once per load at least, so it doesn't fail completely silently and doesn't spam the console with errors.

But @sindresorhus perhaps can give better advice on expected error handling.

fregante · 2018-04-06T07:53:00Z

source/features/add-timezone-to-hovercard.js

+	observer.observe(document.querySelector('div.Popover-message'), {
+		attributes: true,
+		childList: true
+	});


I think we can listen to it directly:

delegate('[data-hydro-view]', 'view', addTime)

The only annoyance is that view is fired after 500ms, for whatever reason:

setTimeout(function() { if (document.body && document.body.contains(e)) { var t = a.query(e, "[data-hovercard-tracking]").getAttribute("data-hovercard-tracking"); if (t) { var n = { event_type: "user-hovercard-load" }; n.dimensions = JSON.parse(t), Yr(n) } var r = a.query(e, "[data-hydro-view]"); i.fire(r, "view") } }, 500)

The only annoyance is that view is fired after 500ms, for whatever reason

I think this won't be an issue if we cache the requests.

Ah yeah I think we might need a way to make it faster?

jorgegonzalez · 2018-04-09T14:25:15Z

So @bfred-it, you suggested that we only show the local time and remove the timezone abbreviation, correct?

I'll start working on removing moment-timezone soon.

jorgegonzalez · 2018-04-09T16:49:02Z

I'm not sure why this is happening.

jorgegonzalez · 2018-04-09T16:52:08Z

source/features/add-timezone-to-hovercard.js

+			const location = select('.Popover .octicon-location').nextSibling.textContent.trim();
+			const timeZone = await getTimezone(location);
+
+			if (timeZone) {


There's an off by one issue here that I haven't figured out.

What do you mean?

fregante · 2018-04-09T16:52:22Z

I'm not sure why this is happening.

Probably because GitHub notices you're hovering the same item over and over and doesn't update the item... but it still fires the "view" event. You'll just have to do a !select.exists(...octicon clock...) at the start of the async function

jorgegonzalez · 2018-04-09T16:53:00Z

source/features/add-timezone-to-hovercard.js

+
+		const now = new Date(Date.now());
+
+		if (document.body.hasAttribute(username)) {


There's probably a better way of doing this.

jorgegonzalez · 2018-04-09T17:32:57Z

Should work now.

fregante · 2018-04-09T17:41:03Z

When you're a good point you can remove "WIP" from the title and I'll review again

fregante · 2018-04-16T09:23:13Z

Requesting reviews because this adds two API calls to Google servers and new permissions.

They dropped the `view` event

jorgegonzalez · 2018-04-16T16:32:00Z

source/features/add-time-to-hovercard.js

@@ -60,5 +63,5 @@ async function updateHovercard() {
 }

 export default async function () {
-	delegate('[data-hydro-view]', 'view', updateHovercard);
+	observeEl('.Popover-message', updateHovercard);


@bfred-it Curious, why did you make this switch back?

They dropped the view event

sindresorhus · 2018-04-17T05:03:46Z

Seems to work great. Can we include the timezone too? I'm only seeing this:

I would like to see 1:02 => 1:02 +5 or something. We should also show if the time is in a different day than my day.

sindresorhus · 2018-04-17T05:04:10Z

The time should not be faded in if it's already cached. Then we should just show it right away.

fregante · 2018-04-17T05:28:33Z

I would like to see 1:02 => 1:02 +5 or something

@sindresorhus should the timezone be relative to GMT (whatever we get from Intl) or relative to me? "Sindre is -1 from me", not "+6"

We should also show if the time is in a different day than my day.

How do you display that though? Usually it's written as "+1" but you're asking to also show the timezone, so that'd be confusing.

1:02 +5 tomorrow?
1:02 +5 wednesday?

The time should not be faded in if it's already cached. Then we should just show it right away.

Thanks to these lines, there's no way to tell... other than manually tracking the time it took to getTimezoneOffset()

sindresorhus · 2018-04-17T06:05:11Z

should the timezone be relative to GMT (whatever we get from Intl) or relative to me? "Sindre is -1 from me", not "+6"

Relative to UTC.

How do you ]display that though? Usually it's written as "+1" but you're asking to also show the timezone, so that'd be confusing.

We would be explicit then:

1:02 (Tomorrow) UTC+5?

Thanks to these lines, there's no way to tell... other than manually tracking the time it took to getTimezoneOffset()

Could just make it return a "tuple":

return [isCached, locationOffsets.get(location)];

hkdobrev · 2018-04-17T07:09:33Z

I was thinking of just showing the local time without a timezone indicator so let's say person A is in Italy and person B is in Greece (1 hour time difference). So if it is 16:00 in Italy and Person A hovers on Person B, they will see 17:00. We'll need a timezone indicator if we want to refer to an absolute timestamp like a rocket launch window. If we refer to a local time somewhere, it should be like when you ask Google what's the time somewhere and Google shows you the local time with no timezone modifier.

sindresorhus · 2018-04-17T07:20:25Z

@hkdobrev Google does show the timezone:

fregante · 2018-04-17T08:57:22Z

All options are valid, we just have to decide what's useful in this context.

We could show the absolute time, 9:19 GMT+2 Tuesday
We could show the relative time to us, 9:19 (+7 hours away) and display the full date/time/zone in the title attribute. GH shows relative dates this almost everywhere.

If we answer this question we might get closer to what we should show:

Why would I want to know the time for a specific person?

I would want to know:

Is person at work? 9:19 answers this question
How far (time) is person from me? -16h would show we're pretty far and will give me a reusable number to my mind (i.e. I don't have to use the popup)

To counter my own point, I made this a while ago. It uses absolute times: https://time.bfred.it/

sindresorhus · 2018-04-21T03:57:06Z

We could show the relative time to us, 9:19 (+7 hours away) and display the full date/time/zone in the title attribute. GH shows relative dates this almost everywhere.

Let's go with this. I thought I needed the UTC time zone, but I can't think of what I would use it for when I know how many hours away from me and the local time. Like you said, we can display the full info in the title attribute.

If it's a different day, maybe we can also include the name of the day here, to make it quicker to scan: 9:19 Tuesday (-7 hours away)?

yakov116 · 2018-05-13T12:15:20Z

Giving this a bump...

jorgegonzalez · 2018-05-15T23:25:23Z

Sorry, I've been pretty busy recently. I'll try and finish this PR soon. Anyone else is welcome to push to my branch too!

sindresorhus · 2018-11-07T07:50:14Z

@jorgegonzalez Do you think you will finish this at some point or should we close? :)

jorgegonzalez · 2018-11-07T19:14:30Z

@sindresorhus let me try and finish it this weekend. I've been swamped since I started working.

jorgegonzalez and others added 2 commits April 5, 2018 23:28

Add local time to hovercards

9c71210

Update add-timezone-to-hovercard.js

223dd16

jorgegonzalez and others added 2 commits April 6, 2018 00:36

Add clock icon left of timezone

e3e9f76

Delete whitespace

3b024d1

fregante requested changes Apr 6, 2018

View reviewed changes

jorgegonzalez and others added 3 commits April 6, 2018 20:40

Merge branch 'master' into master

c90e084

Implement review changes

3397f1d

Update add-timezone-to-hovercard.js

7f1fd32

jorgegonzalez and others added 2 commits April 9, 2018 12:50

Implement simple cache

fbcd968

Remove moment-timezone

53e1aff

jorgegonzalez commented Apr 9, 2018

View reviewed changes

jorgegonzalez added 3 commits April 9, 2018 13:13

Fix issue with duplicate timestamps

7a07372

Merge branch 'master' of https://github.com/jorgegonzalez/refined-github

4592836

Change attribute keys

0fbbc92

jorgegonzalez added 2 commits April 9, 2018 13:35

Fix offset calculation and only show hours and minutes

30db8ee

Update add-timezone-to-hovercard.js

76ab1bf

fregante self-assigned this Apr 9, 2018

fregante requested review from sindresorhus and hkdobrev April 16, 2018 09:18

fregante added 2 commits April 16, 2018 17:36

Return to MutationObserver

a7de462

They dropped the `view` event

Make readme description passive

c6d52c2

fregante removed the needs readme update label Apr 16, 2018

Typo

07cbd08

jorgegonzalez commented Apr 16, 2018

View reviewed changes

fregante removed their assignment Apr 17, 2018

jorgegonzalez and others added 3 commits April 18, 2018 08:04

Merge branch 'master' into master

7a0b0ac

Whitespace

3eb59c0

Don't fade in if offset was cached

d84f13e

sindresorhus force-pushed the master branch from 17f900d to 744e99a Compare May 18, 2018 16:31

fregante mentioned this pull request Jul 9, 2018

Show due date on closed milestones #1399

Closed

jorgegonzalez added 2 commits August 12, 2018 22:33

Merge branch 'master' into master

d40bcea

Merge branch 'master' into master

fb10f69

jorgegonzalez closed this Nov 13, 2018

kiprasmel mentioned this pull request Oct 31, 2019

Add local time to hover cards based on user location #1233

Closed


		const now = new Date(Date.now());

		if (document.body.hasAttribute(username)) {

Add local time to hover cards based on user location #1249

Add local time to hover cards based on user location #1249

Conversation

jorgegonzalez commented Apr 6, 2018 • edited by hkdobrev

fregante commented Apr 6, 2018 • edited

fregante commented Apr 6, 2018 • edited

fregante commented Apr 6, 2018

fregante commented Apr 6, 2018 • edited

jorgegonzalez commented Apr 6, 2018

jorgegonzalez commented Apr 6, 2018

fregante commented Apr 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fregante Apr 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgegonzalez commented Apr 9, 2018 • edited

jorgegonzalez commented Apr 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fregante commented Apr 9, 2018 • edited

Choose a reason for hiding this comment

jorgegonzalez commented Apr 9, 2018

fregante commented Apr 9, 2018

fregante commented Apr 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Apr 17, 2018

sindresorhus commented Apr 17, 2018

fregante commented Apr 17, 2018 • edited

sindresorhus commented Apr 17, 2018

hkdobrev commented Apr 17, 2018

sindresorhus commented Apr 17, 2018

fregante commented Apr 17, 2018 • edited

sindresorhus commented Apr 21, 2018 • edited

yakov116 commented May 13, 2018

jorgegonzalez commented May 15, 2018

sindresorhus commented Nov 7, 2018

jorgegonzalez commented Nov 7, 2018

jorgegonzalez commented Apr 6, 2018 •

edited by hkdobrev

fregante commented Apr 6, 2018 •

edited

fregante commented Apr 6, 2018 •

edited

fregante commented Apr 6, 2018 •

edited

fregante commented Apr 6, 2018 •

edited

fregante Apr 6, 2018 •

edited

jorgegonzalez commented Apr 9, 2018 •

edited

fregante commented Apr 9, 2018 •

edited

fregante commented Apr 16, 2018 •

edited

fregante commented Apr 17, 2018 •

edited

fregante commented Apr 17, 2018 •

edited

sindresorhus commented Apr 21, 2018 •

edited