Skip to content

🔨Refactor /app/pages/GitHub/index.js #2657

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Oct 27, 2019
Merged

🔨Refactor /app/pages/GitHub/index.js #2657

merged 3 commits into from
Oct 27, 2019

Conversation

@nobioma1
Copy link
Contributor

@nobioma1 nobioma1 commented Oct 4, 2019 •
edited
Loading

What kind of change does this PR introduce?

@Saeris @christianalfoni

What is the current behavior?

  • /app/pages/GitHub/index.tsx was using inject and hooksObserver from app/componentConnectors

What is the new behavior?

  • Using useOvermind from app/overmind

What steps did you take to test this?

Ran yarn test and yarn lint

Checklist

  • Documentation
  • Testing
  • Ready to be merged
  • Added myself to contributors table

@vercel vercel bot temporarily deployed to staging October 4, 2019 21:50 Inactive
@vercel
Copy link

vercel bot commented Oct 4, 2019 •
edited
Loading

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://codesandbox-client-git-fork-nobioma1-refactor-dashboard-github.codesandbox1.now.sh

@Saeris Saeris added Hacktoberfest 🔨 Refactor 🧠 Overmind Indicates that this is related to the app's State Management labels Oct 8, 2019
Saeris
Saeris suggested changes Oct 15, 2019
View reviewed changes
Copy link
Contributor

@Saeris Saeris 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! Just make sure that all import references for this component are updated to use the new named export instead of the old default export!

@lbogdan lbogdan temporarily deployed to pr2657 October 15, 2019 22:49 Inactive
@nobioma1
Copy link
Contributor Author

nobioma1 commented Oct 15, 2019

Thank you @Saeris. I went through but can't seem to find a default import for the component 'Github'.

@CompuIves
Copy link
Member

CompuIves commented Oct 17, 2019

Heyo! I think the default import is here: https://github.com/CompuIves/codesandbox-client/blob/master/packages/app/src/app/pages/index.tsx#L43-L45. This one should be changed.

CompuIves
CompuIves requested changes Oct 17, 2019
View reviewed changes
Copy link
Member

@CompuIves CompuIves left a comment

Choose a reason for hiding this comment

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

Need to change default export.

@kiliman kiliman mentioned this pull request Oct 18, 2019
Closed
84 tasks
@nobioma1 nobioma1 requested review from CompuIves and Saeris October 27, 2019 14:58
CompuIves
CompuIves approved these changes Oct 27, 2019
View reviewed changes
Copy link
Member

@CompuIves CompuIves 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 now!

@CompuIves
Copy link
Member

CompuIves commented Oct 27, 2019

Thanks for the PR 😄

@CompuIves CompuIves merged commit e5bd388 into codesandbox:master Oct 27, 2019
@MichaelDeBoey MichaelDeBoey mentioned this pull request Nov 21, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@CompuIves CompuIves CompuIves approved these changes

@Saeris Saeris Awaiting requested review from Saeris

Assignees

No one assigned

Labels

🧠 Overmind Indicates that this is related to the app's State Management 🔨 Refactor

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@nobioma1 @CompuIves @Saeris @lbogdan