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

Changing forceReRender to an array #102

Merged
merged 2 commits into from
Apr 6, 2021

Conversation

pedroapfilho
Copy link
Contributor

@pedroapfilho pedroapfilho commented Apr 5, 2021

Fixes #98

This makes the forceReRender an array, so we can add multiple things to the useEffect inside the component.

This enables us to add things like currency, amount, order and etc to the forceReRender, so it is always correct at the moment that the user clicks the button to Checkout.

This is a breaking change.

@pedroapfilho pedroapfilho requested a review from a team as a code owner April 5, 2021 16:41
@@ -108,7 +108,7 @@ describe("<PayPalButtons />", () => {
<button onClick={() => setAmount(amount + 1)}>
Update Amount
</button>
<PayPalButtons forceReRender={amount} />
<PayPalButtons forceReRender={[amount]} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @pedroapfilho! This looks great 💯

It would be nice to have this unit test make sure forceRerender works with watching multiple variables. Could you update this to watch another variable? Something like:

<PayPalButtons forceReRender={[amount, currency]} />

And then test currency the same way amount is being tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sure I can! It was lazy from my side to only update these tests, I'll definitely add more!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added!

One thing that I couldn't do was to run the project locally. It seems that I have some problems with husky. Do you have any place where you documented what you did on the update? And how you run this project?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding the additional tests!

One thing that I couldn't do was to run the project locally.

What operating system are you using? I'm on a mac and the following steps should work to get husky working:

  1. rm -rf node_modules
  2. npm install
  3. npm start or npm run validate

Let me know if that helps.

Copy link
Contributor

Choose a reason for hiding this comment

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

I went ahead and merged the PR. The CI failure was just due to the commit message format. I fixed it with the squash and merge option.

Thanks for contributing @pedroapfilho! 💯 🥇 😄

@@ -100,15 +100,19 @@ describe("<PayPalButtons />", () => {
});
});

test("should re-render Buttons when props.forceReRender changes", async () => {
test("should re-render Buttons when any item from props.forceReRender changes", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

@gregjopa gregjopa merged commit 362871a into paypal:main Apr 6, 2021
gregjopa pushed a commit that referenced this pull request Apr 11, 2021
BREAKING CHANGE: forceReRender prop now accepts an array.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] forceRerender as an array
2 participants