Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add link to forked repo below the original #2141

Closed
wants to merge 24 commits into from

Conversation

Projects
None yet
2 participants
@jerone
Copy link
Contributor

commented Jun 9, 2019

Description

Add links to the forked repos below the original repo. For users and organizations.

Closes

Closes #1129
Supersedes attempt 1 #1108
Supersedes attempt 2 #2105

Screenshots

Original repo:
image

User forked repo:
image

Organization forked repo:
image

Hover card:
image
Feature removed

Test

  1. Open any repo and click on the Fork button.
  2. Fork the repo.
  3. Go back to the original repo.
  4. Below the repo name, a link to your fork will presented.
@jerone

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2019

@bfred-it Before I going to fix every linting error, can you review if this PR satisfies the way you like to see this implemented.

@jerone

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2019

  • When you visit a repo
    1. it checks for cached forks
    2. and validates if forks still exists
    3. and add fork links.
  • When you visit a fork (user or org)
    1. it caches the fork.
  • When you open the fork dialog
    1. it caches all forks.
@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented Jun 9, 2019

Looks about right, the only issue is ensuring that the forked page you visit is actually your fork, so you might need to look for the Settings tab or something like that to double check.

@jerone jerone changed the title Add link to forked repo below the original [WIP] Add link to forked repo below the original Jun 9, 2019

@jerone jerone changed the title [WIP] Add link to forked repo below the original Add link to forked repo below the original Jun 9, 2019

@jerone

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2019

@bfred-it Ok, this is ready for review. 👀

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

This comment has been minimized.

Copy link
Collaborator

commented Jun 9, 2019

This can be cleaned up further, do a couple of simplification passes

jerone added some commits Jun 9, 2019

@jerone

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2019

@bfred-it Feedback round 1 done.

@bfred-it
Copy link
Collaborator

left a comment

Still needs a pass or two. Merge the short functions that are used once or always together, you’ll find further simplification possible. Can be brought to 110 likes probably

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

jerone added some commits Jun 10, 2019

@jerone

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

Brought it back to 122 LOC.

@jerone

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

Found one issue, that I'm not sure is related to this new feature...

  1. Go to the global PR's.
  2. Find any PR repo that you know has forks.
  3. See no forks being loaded!
  4. Navigate to issues or any other page on the repo (or refresh page) and it works.

I can only reproduce the issue via above route. Any other route, the forks are loaded correctly.

@jerone

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

@bfred-it All feedback resolved.

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

jerone added some commits Jun 12, 2019

@jerone

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

@bfred-it Feedback processed again.


async function validateFork(repo: string): Promise<boolean> {
const response = await fetch(location.origin + '/' + repo,
{

This comment has been minimized.

Copy link
@bfred-it

bfred-it Jun 13, 2019

Collaborator

Not the suggested indentation

}

function watchForkDialog(): void {
const forkDialog = select<HTMLElement>('details-dialog[src*="/fork"]')!;

This comment has been minimized.

Copy link
@bfred-it

bfred-it Jun 13, 2019

Collaborator

All the feedback I sent before applies to other parts of the file as well. I can’t review every single line. This PR has been sloppy. Please clean it up and open a new PR

This comment has been minimized.

Copy link
@jerone

jerone Jun 13, 2019

Author Contributor

Webpack compiles, lint succeeds, the PR does exactly what it should do, in the way you specified it should be done, without code style guide. I try to follow other features syntax, but you have a very opinionated way of writing code. I'm not saying that that is bad, and the feedback is great.

See #2153

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.