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

POC for #2379: GithubInfo Custom Hook to remove duplicate Github Info code #2764

Merged
merged 1 commit into from
Feb 2, 2022
Merged

POC for #2379: GithubInfo Custom Hook to remove duplicate Github Info code #2764

merged 1 commit into from
Feb 2, 2022

Conversation

HyperTHD
Copy link
Contributor

@HyperTHD HyperTHD commented Jan 28, 2022

Issue This PR Addresses

This PR addresses #2379

Type of Change

  • Bugfix: Change which fixes an issue
  • New Feature: Change which adds functionality
  • Documentation Update: Change which improves documentation
  • UI: Change which improves UI

Description

As part of #2379, We needed a way to reduce the amount of duplicate code used in our github info related components. This PR creates a new custom hook that deals with maintaining the amount of props we were passing into the github info components. As such, there's 0 props on all the components except for the users component since it needs the avatarSize prop to adjust for the user icon size. I've also left the smaller function that uses a regex to sort out the urls since they all differ between the github info components. I can add them into the hook if needed.

The custom hook is wrapped inside the "Posts" component since it's the main component all the other components live inside and thus will be the only ones that need access to the context values provided.

Checklist

  • Quality: This PR builds and passes our npm test and works locally
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
  • Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

@HyperTHD HyperTHD added this to the 2.6 Release milestone Jan 28, 2022
@HyperTHD HyperTHD self-assigned this Jan 28, 2022
@gitpod-io
Copy link

gitpod-io bot commented Jan 28, 2022

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but let's shift the URL parsing into the provider.

src/web/src/components/Posts/Post.tsx Outdated Show resolved Hide resolved
@aserputov
Copy link
Contributor

@HyperTHD hm, something wrong with prerendering "/about" page . 🤔

@aserputov aserputov linked an issue Feb 1, 2022 that may be closed by this pull request
humphd
humphd previously approved these changes Feb 1, 2022
@AmasiaNalbandian
Copy link
Contributor

Running another set of tests for the workflow just to be sure, but the e2e is failing. Looks good on deployment though.

@HyperTHD
Copy link
Contributor Author

HyperTHD commented Feb 1, 2022

e2e tests breaking on a page I never touched is disappointing. Will investigate this

@humphd
Copy link
Contributor

humphd commented Feb 1, 2022

cc @TueeNguyen

> Build error occurred
Error: Export encountered errors on following paths:
	/about
    at /frontend/node_modules/next/dist/export/index.js:499:19

This is the same bug that was reported in Slack with docker builds on master.

I'd say master is currently broken, cc sheriffs: @sirinoks, @aserputov. We may need to figure out what needs to get downgraded or backed out. My first guess is a recent update of next.js? This needs research, as it will start to break everyone.

@aserputov
Copy link
Contributor

aserputov commented Feb 2, 2022

@HyperTHD now it's working, can you please rebase this and we will merge it 🙂

@HyperTHD
Copy link
Contributor Author

HyperTHD commented Feb 2, 2022

@HyperTHD now it's working, can you please rebase this and we will merge it 🙂

Rebased and ready to be looked at

@aserputov
Copy link
Contributor

I still see five commits, can you re-check it please ?

@aserputov aserputov requested a review from humphd February 2, 2022 03:39
aserputov
aserputov previously approved these changes Feb 2, 2022
@aserputov
Copy link
Contributor

@humphd Can I just 'squash and merge' or the PR owner should do it ?

@aserputov aserputov requested a review from manekenpix February 2, 2022 04:28
humphd
humphd previously approved these changes Feb 2, 2022
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HyperTHD can you squash, rebase, and merge this please?

@aserputov
Copy link
Contributor

@HyperTHD Also, I have a question about #2380. Are we going to create another PR for it?

@HyperTHD HyperTHD merged commit 95b8850 into Seneca-CDOT:master Feb 2, 2022
@HyperTHD HyperTHD deleted the GithubInfoHook branch February 2, 2022 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove duplicate code between Issues, Commits, Repos, PullRequest
5 participants