Add tree links to issue timestamps #741

jgierer12 · 2017-10-18T20:32:20Z

Fixes #732

I often find myself browsing through old issues, and sometimes when reading a discussion I think to myself, "I wonder what that part of the code looked like while that was being discussed". Right now it's super tedious to find out, because you have to remember the comment date, then page through the commits until you find something roughly at that date, and then find the "view files at" button, etc.

It would be super awesome if there was an extra little icon button next to comments that let you "jump to the repo at this point on time"!

@ianstormtaylor @sindresorhus is this what you had in mind functionality-wise?

This still needs a lot of work:

I don't really know how to fit this into there styling-wise. ~~Currently making a few concepts~~ See Add tree links to issue timestamps #741 (comment), Add tree links to issue timestamps #741 (comment)
Currently, the button is added to every timestamp link on issue/PR pages (including timestamps for activities etc.). This might be useful but only works if it's really small and non-intrusive
Since the target commit is loaded dynamically after clicking the button, it's not actually a link. This means that things like middle-clicking/ctrl-clicking to open in a new tab don't work. Should we open in a new tab by default?
Still need to figure out ~~GitHub Enterprise support (cc @busches) and~~ private repo support
Messy code needs to be cleaned up

tanmayrajani · 2017-10-18T20:35:30Z

@jgierer12 Oh shoot, I didn't notice you assigned it to yourself and worked on it. I worked on this too 😬. What should I do now?

tanmayrajani · 2017-10-18T20:44:35Z

I thought a lot and sent duplicate pull anyway mentioning the same. You can have a look. 😄

jgierer12 · 2017-10-18T20:53:58Z

Thanks, sorry again!

jgierer12 · 2017-10-18T21:02:42Z

Design concepts I have so far:

A 'browse files' button like on single commit pages (this is probably the closest thing in native GitHub to these timestamp links)

Pros: unambiguous, consistent with GH's native design, easy to add loading/error styles
Cons: big, intrusive and attention-grabbing
Leave it as a link button (maybe make it grey instead)

Pros: Similar to button but a bit smaller
Cons: Still pretty large
An icon button. Maybe the eye icon?

Pros: small, unintrusive
Cons: hard to guess what it does, bit harder to do loading/error styles

yakov116 · 2017-10-18T21:04:04Z

I like option 3.

Can you show it with more detail on how it would show on the page

ianstormtaylor · 2017-10-18T21:06:13Z

I think it was the Option 3 icon button, but with this icon and tooltip it would be discoverable enough: (GitHub uses this icon already for this behavior it seems, so it would match.)

Ideally in the same "style" as this icon:

jgierer12 · 2017-10-18T21:46:49Z

Ok, I updated the PR with icon buttons for now:

Idle	Loading	Error

fregante · 2017-10-19T06:55:27Z

src/content.js

@@ -645,6 +646,7 @@ async function onDomReady() {
 			if (pageDetect.isPR() || pageDetect.isIssue()) {
 				safely(linkifyIssuesInTitles);
 				observeEl('.new-discussion-timeline', addOPLabels, {childList: true, subtree: true});
+				observeEl('.new-discussion-timeline', addTimestampTreeLinks, {childList: true, subtree: true});


Since these are identical, can you merge them?

fregante · 2017-10-19T06:58:29Z

src/libs/timestamp-tree-links.js

+				return;
+			}
+
+			location.href = `https://github.com/${ownerName}/${repoName}/tree/${sha}`;


Drop protocol and hostname to support GHEnterprise

fregante · 2017-10-19T07:00:23Z

src/libs/timestamp-tree-links.js

+		const timestampValue = select('relative-time', timestampLink).attributes.datetime.value;
+
+		const openTree = async ({target}) => {
+			target.replaceChild(icons.clock(), target.firstChild);


All replaceChild instances can be replaced by firstChild.replaceWith

fregante · 2017-10-19T07:01:54Z

src/libs/timestamp-tree-links.js

+export default async () => {
+	const newTimestamps = select.all(`.new-discussion-timeline .timestamp:not(.refined-github-comment-browse-files)`);
+
+	newTimestamps.forEach(async timestampLink => {


async is not needed here, this loop can be replaced with for of

fregante · 2017-10-19T07:05:45Z

src/libs/timestamp-tree-links.js

+const {ownerName, repoName} = getOwnerAndRepo();
+
+const getSHABeforeTimestamp = async timestamp => {
+	const url = `https://api.github.com/repos/${ownerName}/${repoName}/commits?until=${timestamp}&per_page=1`;


If we’re positive we can’t do this without the API, we still need to support /api instead of the subdomain, for GHE. You might want to pack it up as a getApiUrl function because we’ll probably need the same logic again in the future.

It would match "edited" times and it would fail.

fregante · 2017-10-19T12:13:29Z

Note: if we settle for the commits page, we can forgo the API call and just load this page:

https://github.com/sindresorhus/refined-github/commits/master?until=2017-08-09T22:24:21Z

"Timestamp tree link" didn't sound right. "Time Machine" here should remind of macOS' Time Machine or just "going back in time" Name suggestions welcome

fregante · 2017-11-01T12:10:11Z

This is the tooltip (updated to show above the icon for consistency):

+ correct icon alignment

sindresorhus · 2017-11-06T06:45:14Z

@bfred-it Can you fix the merge conflict? Is this PR done?

fregante · 2017-11-06T06:51:01Z

Yep it's done

sindresorhus · 2017-11-06T07:00:37Z

src/libs/api.js

+export default endpoint => {
+	const api = location.hostname === 'github.com' ? 'https://api.github.com/' : `${location.origin}/api/`;
+	return fetch(api + endpoint).then(res => res.json());
+};


This file looks unused?

Currently, yes, but it's not being included at the moment. I feel it's a start for whenever the API materializes.

sindresorhus · 2017-11-06T07:02:18Z

extension/content.css

+	padding: 0;
+}
+:root .rgh-timestamp-button .octicon {
+	vertical-align: sub;


I think this should be sub => middle. It looks slightly vertically unaligned now.

Actually for some reason this isn't being applied at all so it defaults to middle (which isn't aligned)

baseline is the default, not middle.

When I manually applied middle in devtools it looked ok.

@jgierer12

Created by @jgierer12 and @bfred-it

sindresorhus · 2017-11-06T09:25:54Z

Great work @jgierer12 and @bfred-it. This is a very useful feature. And thanks for suggesting it @ianstormtaylor 👌

DrewML · 2017-11-06T21:55:33Z

Just noticed this feature while looking through a giant PR at work, and was so excited when I saw it. Thanks for doing this!

Link to tree at timestamps

9827f52

jgierer12 added do not merge yet enhancement labels Oct 18, 2017

tanmayrajani mentioned this pull request Oct 18, 2017

Add "Link to repo at time of issue comment" #742

Closed

Add basic styles

91a4984

Only fetch one commit

a903e1f

Change buttons to icons

c1cc1b8

jgierer12 changed the title ~~[WIP] Add tree links to issue timestamps (#732)~~ [WIP] Add tree links to issue timestamps Oct 18, 2017

fregante reviewed Oct 19, 2017

View reviewed changes

fregante previously requested changes Oct 19, 2017

View reviewed changes

fregante added 8 commits October 19, 2017 02:16

Use .catch instead of try/catch

299a27a

Use replaceWith instead of replaceChild

bbf160b

Use for-of instead of forEach

1ce91a8

Use Node.after instead of insertAdjacentElement

bbf33b1

Extract function from loop

e413bb9

Remove github.com from URL for GHE support

b3c3bd7

Limited to one timestamp per comment

3fb694f

It would match "edited" times and it would fail.

Merge with addOPLabels observer + limit scope

10f7fec

fregante force-pushed the timestamp-tree-links branch 2 times, most recently from 8c005f5 to 580e563 Compare October 19, 2017 08:46

fregante added 2 commits October 19, 2017 16:49

Improved style

580e563

Add GHE API support

e90b041

Plain API-less link 🎉

7041a9e

fregante force-pushed the timestamp-tree-links branch from 95ed917 to 7041a9e Compare October 31, 2017 08:45

fregante added 2 commits November 1, 2017 11:41

Renamed

15d8e1d

"Timestamp tree link" didn't sound right. "Time Machine" here should remind of macOS' Time Machine or just "going back in time" Name suggestions welcome

Support all repos + GHE

0b7674a

fregante removed the do not merge yet label Nov 1, 2017

fregante added 2 commits November 1, 2017 20:08

Added tooltip on hover

42dd525

Added to readme

668182e

fregante force-pushed the timestamp-tree-links branch from 426c437 to 668182e Compare November 1, 2017 12:09

hkdobrev approved these changes Nov 1, 2017

View reviewed changes

fregante added 3 commits November 2, 2017 12:39

Move tooltip to above the link for consistency

8c5b2f2

+ correct icon alignment

Moved file to features folder

6b8d191

Merge remote-tracking branch 'origin/master' into timestamp-tree-links

665d83d

fregante added 2 commits November 6, 2017 00:50

Merge branch 'master' into timestamp-tree-links

758548e

Merge branch 'master' into timestamp-tree-links

018b28b

sindresorhus changed the title ~~[WIP] Add tree links to issue timestamps~~ Add tree links to issue timestamps Nov 6, 2017

Update content.css

59784e2

sindresorhus reviewed Nov 6, 2017

View reviewed changes

Update icons.js

d360b5c

fregante approved these changes Nov 6, 2017

View reviewed changes

Align the button

f210e55

sindresorhus merged commit 235f528 into refined-github:master Nov 6, 2017

sindresorhus pushed a commit that referenced this pull request Nov 6, 2017

Add tree links to issue timestamps (#741)

8d95fc3

Created by @jgierer12 and @bfred-it

fregante mentioned this pull request Nov 6, 2017

Fix subpixel rendering Chrome bug in tooltips' triangles #797

Merged

fregante mentioned this pull request Nov 14, 2017

Show tags in commit history #807

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tree links to issue timestamps #741

Add tree links to issue timestamps #741

jgierer12 commented Oct 18, 2017 •

edited

tanmayrajani commented Oct 18, 2017

tanmayrajani commented Oct 18, 2017

jgierer12 commented Oct 18, 2017

jgierer12 commented Oct 18, 2017

yakov116 commented Oct 18, 2017

ianstormtaylor commented Oct 18, 2017 •

edited

jgierer12 commented Oct 18, 2017

fregante Oct 19, 2017

fregante Oct 19, 2017

fregante Oct 19, 2017

fregante Oct 19, 2017

fregante Oct 19, 2017

fregante commented Oct 19, 2017 •

edited

fregante commented Nov 1, 2017 •

edited

sindresorhus commented Nov 6, 2017

fregante commented Nov 6, 2017

sindresorhus Nov 6, 2017

fregante Nov 6, 2017

sindresorhus Nov 6, 2017

fregante Nov 6, 2017

sindresorhus Nov 6, 2017

sindresorhus Nov 6, 2017

sindresorhus commented Nov 6, 2017

DrewML commented Nov 6, 2017

Add tree links to issue timestamps #741

Add tree links to issue timestamps #741

Conversation

jgierer12 commented Oct 18, 2017 • edited

tanmayrajani commented Oct 18, 2017

tanmayrajani commented Oct 18, 2017

jgierer12 commented Oct 18, 2017

jgierer12 commented Oct 18, 2017

yakov116 commented Oct 18, 2017

ianstormtaylor commented Oct 18, 2017 • edited

jgierer12 commented Oct 18, 2017

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 commented Oct 19, 2017 • edited

fregante commented Nov 1, 2017 • edited

sindresorhus commented Nov 6, 2017

fregante commented Nov 6, 2017

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

sindresorhus commented Nov 6, 2017

DrewML commented Nov 6, 2017

jgierer12 commented Oct 18, 2017 •

edited

ianstormtaylor commented Oct 18, 2017 •

edited

fregante commented Oct 19, 2017 •

edited

fregante commented Nov 1, 2017 •

edited