Skip to content

Refactored 🧠 Overmind Hacktober | patron/subscription: /app/pages/Patron/PricingModal/PricingChoice/ChangeSubscription/index.ts #2654

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 1 commit into from
Oct 28, 2019
Merged

Refactored 🧠 Overmind Hacktober | patron/subscription: /app/pages/Patron/PricingModal/PricingChoice/ChangeSubscription/index.ts #2654

merged 1 commit into from
Oct 28, 2019

Conversation

@NileshPatel17
Copy link
Contributor

@NileshPatel17 NileshPatel17 commented Oct 4, 2019 •
edited
Loading

What kind of change does this PR introduce?

This is part of the requested refactor for hacktoberfest mentioned in #2621.

What is the current behavior?

Previously it was utilizing inject and hooksObserver from app/componentConnectors.
#2621

What is the new behavior?

Prefers useOvermind over inject

What steps did you take to test this?

  • yarn:lint: There were several linting errors, but none relevant to the scope of this PR
  • yarn:test: Ran successfully

Checklist

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

@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-cl-git-fork-nileshpatel17-refactor-patron-su-a61ba0.codesandbox1.now.sh

@vercel vercel bot temporarily deployed to staging October 4, 2019 16:58 Inactive
@NileshPatel17 NileshPatel17 changed the title refactor patron/subscription: /app/pages/Patron/PricingModal/PricingChoice/ChangeSubscription/index.ts Refactored 🧠 Overmind Hacktober | patron/subscription: /app/pages/Patron/PricingModal/PricingChoice/ChangeSubscription/index.ts Oct 4, 2019
@NileshPatel17 NileshPatel17 marked this pull request as ready for review October 4, 2019 17:01
@NileshPatel17 NileshPatel17 mentioned this pull request Oct 4, 2019
Closed
84 tasks
jyash97
jyash97 reviewed Oct 4, 2019
View reviewed changes
packages/app/src/app/pages/Patron/PricingModal/PricingChoice/ChangeSubscription/index.tsx Outdated
patron: { tryAgainClicked },
},
} = useOvermind();
const isLoading = isUpdatingSubscription;
Copy link
Contributor

@jyash97 jyash97 Oct 4, 2019

Choose a reason for hiding this comment

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

How about not defining a new variable and directly using isUpdatingSubscription or doing this { isUpdatingSubscription: isLoading, error } at L30?

Copy link
Contributor

@jyash97 jyash97 Oct 4, 2019

Choose a reason for hiding this comment

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

Wdyt @Saeris @christianalfoni ?

Copy link
Contributor Author

@NileshPatel17 NileshPatel17 Oct 4, 2019

Choose a reason for hiding this comment

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

How about not defining a new variable and directly using isUpdatingSubscription or doing this { isUpdatingSubscription: isLoading, error } at L30?

okay, will fix it.

Copy link
Contributor Author

@NileshPatel17 NileshPatel17 Oct 5, 2019

Choose a reason for hiding this comment

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

@jyash97 its fixed, and also rebase commit. let me know if any more changes are required.

@NileshPatel17 NileshPatel17 force-pushed the refactor/patron-subscription branch from 6300027 to 606ec3e Compare October 5, 2019 08:59
@vercel vercel bot temporarily deployed to staging October 5, 2019 08:59 Inactive
@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
packages/app/src/app/pages/Patron/PricingModal/PricingChoice/ChangeSubscription/index.tsx Outdated
signals: any;
}

function ChangeSubscriptionComponent({
Copy link
Contributor

@Saeris Saeris Oct 15, 2019

Choose a reason for hiding this comment

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

Let's change this to an arrow function and just export the component directly instead of declaring then exporting with a different name at the end of the file, ie:

export const ChangeSubscription: React.FC<IChangeSubscriptionProps> = ({

Copy link
Contributor Author

@NileshPatel17 NileshPatel17 Oct 15, 2019

Choose a reason for hiding this comment

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

Done. also verified the import reference across code. no changes are required.

@NileshPatel17 NileshPatel17 force-pushed the refactor/patron-subscription branch from 606ec3e to 10e35a8 Compare October 15, 2019 15:13
@CompuIves
Copy link
Member

CompuIves commented Oct 17, 2019

Heyo! I tested this PR, but it seems like there is a still a syntax error in the component. The arrow (=>) of the arrow function is missing. After adding that this is ready to be merged.

@NileshPatel17 NileshPatel17 force-pushed the refactor/patron-subscription branch from 10e35a8 to 454f07a Compare October 17, 2019 13:20
@NileshPatel17
Copy link
Contributor Author

NileshPatel17 commented Oct 17, 2019

its fixed now. do not know how i missed it. sorry about that.

@NileshPatel17
Copy link
Contributor Author

NileshPatel17 commented Oct 22, 2019

did you get chance to take a look at. let me know if it requires more changes.

@SaraVieira
Copy link
Contributor

SaraVieira commented Oct 22, 2019

Everything looks good, trying to run the integrations again

CompuIves
CompuIves approved these changes Oct 28, 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.

LGTM! :shipit:

@CompuIves CompuIves merged commit 25d3730 into codesandbox:master Oct 28, 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

+2 more reviewers

@jyash97 jyash97 jyash97 left review comments

@Saeris Saeris Saeris requested changes

Reviewers whose approvals may not affect merge requirements

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.

6 participants

@NileshPatel17 @CompuIves @SaraVieira @Saeris @jyash97 @lbogdan