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

Generalize the GitHubInfoProvider #2947

Merged
merged 1 commit into from Feb 17, 2022
Merged

Conversation

dbelokon
Copy link
Contributor

Issue This PR Addresses

Fixes #2946

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
  • Refactoring: Change that refactors code without modifying current behaviour

Description

This PR refactors the GitHubInfo Provider so that we can fit other types of data easier.

Steps to test the PR

  1. Pull my changes (git fetch dbelokon, you need a remote reference to my repo first!)
  2. Go to the commit (git checkout issue-2946)
  3. Run the front-end with any settings you prefer, although I recommend using env.staging because only the front-end is necessary (pnpm dev)

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)

@dbelokon dbelokon added this to the 2.7 Release milestone Feb 16, 2022
@dbelokon dbelokon self-assigned this Feb 16, 2022
@gitpod-io
Copy link

gitpod-io bot commented Feb 16, 2022

Copy link
Contributor

@menghif menghif left a comment

Choose a reason for hiding this comment

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

Can we move the GitHubInfo logic from GenericInfoProvider.tsx to its own component GitHubInfo.tsx?

@dbelokon
Copy link
Contributor Author

Can we move the GitHubInfo logic from GenericInfoProvider.tsx to its own component GitHubInfo.tsx?

Sure!

menghif
menghif previously approved these changes Feb 16, 2022
Copy link
Contributor

@menghif menghif 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!

TueeNguyen
TueeNguyen previously approved these changes Feb 16, 2022
Copy link
Contributor

@TueeNguyen TueeNguyen left a comment

Choose a reason for hiding this comment

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

I just learnt about context here in your PR, I think it looks ok. You're going to add youtubeInfo in following PRs right?

@dbelokon
Copy link
Contributor Author

I just learnt about context here in your PR, I think it looks ok. You're going to add youtubeInfo in following PRs right?

Yeah, YouTubeInfo.tsx would be added in the second PR. I didn't want to do everything in a single PR because it would be overwhelming to review.

I will try to squash and rebase now!

@TueeNguyen
Copy link
Contributor

@dbelokon, let's rebase again and clear this PR!

@TueeNguyen TueeNguyen merged commit 2c88073 into Seneca-CDOT:master Feb 17, 2022
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.

Generalize the GitHubInfo Provider
3 participants