Add `fork-source-link-same-view` feature #3230

yakov116 · 2020-06-14T03:45:30Z

LINKED ISSUES:
Closes default-branch-button on forks: add secondary button to see folder on source repo #2985
TEST URLS:
Any forked repo
SCREENSHOT:

Notes: I "hosted" the function in this feature since I felt that forked-to was getting crowded.

source/features/fork-source-link-same-view.tsx

fregante · 2020-06-14T09:45:46Z

Also does this feature need a screenshot?

Yes, as small as possible around the repo name and fork link, but include the browser’s status bar to show its different. I did something similar for the “fix view link” feature.

Side Note. This does lead to 404 sometimes

Yeah it happens when the fork has a new file. It’s expected but at the same time it means we’re breaking that link.

What if we send a HEAD request to the link to ensure it works before changing the pathname?

yakov116 · 2020-06-14T12:53:12Z

I did something similar for the “fix view link” feature.

I am not sure which feature your talking about.

What if we send a HEAD request to the link to ensure it works before changing the pathname?

Ok I had this idea.
Only if its a single file then check if it exists. I am assuming that usually the fork and the source will have the the same directory's. I can change that if you want.
If it does not exists update the link back to the original link.

At first I awaited the check but that was causing issues with the fork dropdown (When there is more than one fork we make it a drop down).

So I decided not to await it and update the link when api finishes. Its a very simple request and did not take even a second. The worst thing that can happen if someone clicks it before and the file does not exists they will get a 404.

Maybe we should only do this on your own forks?

WDYT?

source/features/fork-source-link-same-view.tsx

yakov116 · 2020-06-14T13:40:00Z

Looks like I missed the boat (Ouch) I thought you wanted to check the fork links too if they point to a 404. Correcting.

fregante · 2020-06-14T13:52:21Z

I thought you wanted to check the fork links too if they point to a 404.

I didn't think of that, but in that case we're not breaking an native link, it's our own feature.

yakov116 · 2020-06-14T13:52:52Z

I thought you wanted to check the fork links too if they point to a 404.

I didn't think of that, but in that case we're not breaking an native link, it's our own feature.

💯

yakov116 · 2020-06-14T17:53:49Z

I timed it to test which one is faster, Api wins.

refined-github.js:8953 fetch: 356.760009765625ms
refined-github.js:8959 api: 132.14990234375ms
refined-github.js:8953 fetch: 694.949951171875ms
refined-github.js:8959 api: 143.827880859375ms
refined-github.js:8953 fetch: 599.36279296875ms
refined-github.js:8959 api: 142.7587890625ms
refined-github.js:8953 fetch: 485.253662109375ms
refined-github.js:8959 api: 3.7099609375ms
refined-github.js:8953 fetch: 451.873779296875ms
refined-github.js:8959 api: 140.218994140625ms

yakov116 · 2020-06-14T17:56:19Z

source/github-helpers/index.ts


 export function getForkedRepo(): string | undefined {
-	return select<HTMLAnchorElement>('.fork-flag a')?.pathname.slice(1);
+	return select<HTMLAnchorElement>('.fork-flag a')?.pathname.slice(1).split('/', 2).join('/');


Suggested change

return select<HTMLAnchorElement>('.fork-flag a')?.pathname.slice(1).split('/', 2).join('/');

return select<HTMLAnchorElement>('.fork-flag .text a')?.textContent!.trim();

I think this would work here too, no?

I guess. Double-check by blaming the line

Looks at the blame and finds a magic comment

Also you could use the same meta tags I used in the isForkedRepo since they're available earlier on the page:

<meta name="octolytics-dimension-repository_parent_nwo" content="sindresorhus/refined-github" />

Suggested change

return select<HTMLAnchorElement>('.fork-flag a')?.pathname.slice(1).split('/', 2).join('/');

return select<HTMLMetaElement>('[name="octolytics-dimension-repository_parent_nwo"]')?.content;

Originally posted by @fregante in #2880

Heh, good job. That's what happens when I review after a merge PR, it gets lost

I think this should be put back?

source/features/fork-source-link-same-view.tsx

Co-authored-by: Fregante <opensource@bfred.it>

source/features/fork-source-link-same-view.tsx

yakov116 · 2020-06-15T18:30:46Z

Also does this feature need a screenshot?

Yes, as small as possible around the repo name and fork link, but include the browser’s status bar to show its different. I did something similar for the “fix view link” feature.

I think all I am missing is a screenshot

Can you point me to it? I am not sure which feature your talking about

fregante · 2020-06-15T19:21:30Z

fix-view-file-link-in-pr

https://github.com/sindresorhus/refined-github/blob/4dabb8ce0539222101d39c5329857dafb88327b6/source/features/enable-file-links-in-compare-view.tsx#L61

yakov116 · 2020-06-15T19:39:55Z

@fregante what program do you use? I have a very hard time doing those kind of pictures.

fregante · 2020-06-15T21:35:41Z

I'm not talking about the composition, I'm talking about including the status bar (with the URL) in the screenshot when you hover the button, like:

fregante · 2020-06-16T13:19:43Z

The screenshot misses the context entirely. I have no idea what that is. At least include the repo name and don’t zoom too much

yakov116 · 2020-06-16T14:05:25Z

Is this good?

source/features/fork-source-link-same-view.tsx

source/features/forked-to.tsx

yakov116 · 2020-06-16T18:37:56Z

Cleared up a bit. Untested. Do you see any issues?

I can test it in about an hour

yakov116 · 2020-06-16T19:37:22Z

Do you see any issues?

Yes forked-to link is always changed not just on isRepoRoot and isSingleFile.

For example on this PR it send me to https://github.com/yakov116/refined-github/pull/HEAD

source/features/forked-to.tsx

Co-authored-by: Fregante <opensource@bfred.it>

source/features/forked-to.tsx

fregante

Felt like those changes weren't needed at all. createLink was fine in that feature

source/features/fork-source-link-same-view.tsx

source/features/forked-to.tsx

fregante · 2020-06-18T11:23:59Z

Doesn't work again 😅 it points to itself, not the fork source

https://github.com/fregante/refined-github/blob/master/.editorconfig

source/features/fork-source-link-same-view.tsx

yakov116 · 2020-06-18T11:41:50Z

Felt like those changes weren't needed at all. createLink was fine in that feature
👍

I love how clean it is now.

@fregante I learn from your coding each day. Thanks!

yakov116 · 2020-06-18T11:46:59Z

Not sure it will get lost here please take a look at https://github.com/sindresorhus/refined-github/pull/3230/files/4003fa22b477050b3db99b86ea84e80cb2fb7741#diff-cecb0456449444f8df0aa134a3ba6540

fregante · 2020-06-18T12:11:06Z

Not sure it will get lost here please take a look at sindresorhus/refined-github/pull/3230/files/4003fa22b477050b3db99b86ea84e80cb2fb7741#diff-cecb0456449444f8df0aa134a3ba6540

It's already here: https://github.com/sindresorhus/refined-github/blob/75422077fa0dc554a2a0b50c9e5701e4a336684f/source/github-helpers/index.ts#L50-L52

fregante · 2020-06-18T12:19:29Z

Testing this feature and forked-to should result into this 👍

fregante · 2020-08-10T11:47:41Z

I think this should also support isEditingFile:

Test link: https://github.com/yakov116/refined-github/edit/upstream/package.json

yakov116 · 2020-08-10T11:50:42Z

@fregante I'm on it

Send it just to blob?

fregante · 2020-08-10T11:53:45Z

Why would you send it to blob instead of leaving it as edit?

yakov116 · 2020-08-10T11:54:56Z

Less code :). I am working on it will have a pr in a few

Add fork-source-link-same-view feature

e29bf5b

fregante reviewed Jun 14, 2020

View reviewed changes

source/features/fork-source-link-same-view.tsx Outdated Show resolved Hide resolved

yakov116 added 2 commits June 14, 2020 08:47

Remove default

2bc94db

add checkIfFileExists

cf9caf3

yakov116 commented Jun 14, 2020

View reviewed changes

source/features/fork-source-link-same-view.tsx Outdated Show resolved Hide resolved

fregante reviewed Jun 14, 2020

View reviewed changes

source/features/fork-source-link-same-view.tsx Outdated Show resolved Hide resolved

Feedback

4003fa2

yakov116 commented Jun 14, 2020

View reviewed changes

fregante added the enhancement label Jun 15, 2020

fregante requested changes Jun 15, 2020

View reviewed changes

yakov116 and others added 3 commits June 15, 2020 10:08

Apply suggestions from code review

7b6af69

Co-authored-by: Fregante <opensource@bfred.it>

Update source/github-helpers/index.ts

0fb6022

Fix build (For now) and add readme (no screenshot yet)

9dc1151

fregante reviewed Jun 15, 2020

View reviewed changes

source/features/fork-source-link-same-view.tsx Outdated Show resolved Hide resolved

Feedback

c42ee6e

yakov116 added 2 commits June 15, 2020 18:04

Add screenshot

cdfdb9a

Remove border from image

3d56ada

yakov116 marked this pull request as ready for review June 15, 2020 22:07

fregante reviewed Jun 16, 2020

View reviewed changes

source/features/fork-source-link-same-view.tsx Outdated Show resolved Hide resolved

yakov116 commented Jun 16, 2020

View reviewed changes

source/features/forked-to.tsx Outdated Show resolved Hide resolved

fregante reviewed Jun 16, 2020

View reviewed changes

source/features/forked-to.tsx Outdated Show resolved Hide resolved

Update source/features/forked-to.tsx

2cba76b

Co-authored-by: Fregante <opensource@bfred.it>

fregante reviewed Jun 16, 2020

View reviewed changes

source/features/forked-to.tsx Outdated Show resolved Hide resolved

yakov116 added 2 commits June 16, 2020 16:34

Dont like it, but it works

5d52acc

doesFileExists to its own file

963e26d

yakov116 commented Jun 17, 2020

View reviewed changes

source/features/forked-to.tsx Outdated Show resolved Hide resolved

Revert forked-to.tsx

f1664f0

fregante reviewed Jun 18, 2020

View reviewed changes

source/features/fork-source-link-same-view.tsx Outdated Show resolved Hide resolved

source/features/forked-to.tsx Outdated Show resolved Hide resolved

Merge line

275f2ac

fregante reviewed Jun 18, 2020

View reviewed changes

source/features/fork-source-link-same-view.tsx Outdated Show resolved Hide resolved

Partial fix

6cc6eac

Restore index.ts

6044764

Use forked repository again

b7461a5

fregante merged commit 6f4ac06 into refined-github:master Jun 18, 2020

yakov116 deleted the fork-source-link-same-view branch June 18, 2020 12:19

yakov116 mentioned this pull request Jul 31, 2020

Preserve current branch in forked-to link when possible #3420

Merged

yakov116 mentioned this pull request Aug 10, 2020

Enable fork-source-link-same-view on editing pages #3450

Merged

	return select<HTMLAnchorElement>('.fork-flag a')?.pathname.slice(1).split('/', 2).join('/');
	return select<HTMLAnchorElement>('.fork-flag .text a')?.textContent!.trim();

	return select<HTMLAnchorElement>('.fork-flag a')?.pathname.slice(1).split('/', 2).join('/');
	return select<HTMLMetaElement>('[name="octolytics-dimension-repository_parent_nwo"]')?.content;

Uh oh!

Add fork-source-link-same-view feature #3230

Add fork-source-link-same-view feature #3230

Conversation

yakov116 commented Jun 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

fregante commented Jun 14, 2020

Uh oh!

yakov116 commented Jun 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yakov116 commented Jun 14, 2020

Uh oh!

fregante commented Jun 14, 2020

Uh oh!

yakov116 commented Jun 14, 2020

Uh oh!

yakov116 commented Jun 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yakov116 Jun 14, 2020

Choose a reason for hiding this comment

Uh oh!

fregante Jun 15, 2020

Choose a reason for hiding this comment

Uh oh!

yakov116 Jun 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fregante Jun 15, 2020

Choose a reason for hiding this comment

Uh oh!

yakov116 Jun 18, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yakov116 commented Jun 15, 2020

Uh oh!

fregante commented Jun 15, 2020

Uh oh!

yakov116 commented Jun 15, 2020

Uh oh!

fregante commented Jun 15, 2020

Uh oh!

fregante commented Jun 16, 2020

Uh oh!

yakov116 commented Jun 16, 2020

Uh oh!

Uh oh!

Uh oh!

yakov116 commented Jun 16, 2020

Uh oh!

yakov116 commented Jun 16, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fregante left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fregante commented Jun 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

yakov116 commented Jun 18, 2020

Uh oh!

yakov116 commented Jun 18, 2020

Uh oh!

fregante commented Jun 18, 2020

Add `fork-source-link-same-view` feature #3230

Add `fork-source-link-same-view` feature #3230

yakov116 commented Jun 14, 2020 •

edited

Loading

yakov116 commented Jun 14, 2020 •

edited

Loading

yakov116 commented Jun 14, 2020 •

edited

Loading

yakov116 Jun 15, 2020 •

edited

Loading

fregante commented Jun 18, 2020 •

edited

Loading

fregante commented Aug 10, 2020 •

edited

Loading

yakov116 commented Aug 10, 2020 •

edited

Loading