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

feat(deps): react@18 support #981

Merged
merged 1 commit into from
Sep 27, 2022

Conversation

chanceaclark
Copy link
Contributor

@chanceaclark chanceaclark commented Sep 20, 2022

What?

🎉 Finally... the time has come... React@18 support! 🎉

Why?

All the necessary libraries have finally been updated to support React@18.

Screenshots/Screen Recordings

Documentation site working as expected

screencapture-localhost-3000-2022-09-20-12_39_59

Testing/Proof

Tested in channel-manager within the Control Panel.


unmount();

rerender(
render(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since I bumped @testing-library/react, their unmount method completely wipes container.innerhtml so we just need to use render instead. rerender is really only useful for updates to props.

Comment on lines +550 to +552
await userEvent.click(button);

await userEvent.click(button);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test was failing with act so switched over to userEvent.

Comment on lines +15 to 25
renderHook(() => useUpdateItems(), {
wrapper: class Wrapper extends Component<PropsWithChildren<unknown>> {
override componentDidCatch(err: unknown) {
error = err;
}
override render() {
return this.props.children;
}
},
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When they ported over renderHook to v13 of @testing-library/react they shipped it with a leaner API. Now we don't get errors with this, but according to testing-library/react-testing-library#991 (comment) this is the best way to achieve the same thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

😵‍💫

Comment on lines +61 to +63
<Small as="span" color="inherit">
Read more
</Small>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screen Shot 2022-09-20 at 12 28 14

Comment on lines +21 to +25
Auto dismiss after 5 seconds.
{/* I hate using a br but the as prop doesn't support div for now */}
{/* TODO: Support div for as prop */}
<br />
<Small as="span">Note: Only valid when used with AlertManager.</Small>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screen Shot 2022-09-20 at 12 36 52

Copy link
Contributor

Choose a reason for hiding this comment

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

:cringe:

Comment on lines +67 to +71
Determines type of z-index to be applied.
{/* I hate using a br but the as prop doesn't support div for now */}
{/* TODO: Support div for as prop */}
<br />
<Small as="span">Types are in order (sticky is low, popover is high).</Small>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screen Shot 2022-09-20 at 12 38 51

@chanceaclark chanceaclark force-pushed the feat/react-18 branch 2 times, most recently from f4f1be7 to 7493844 Compare September 20, 2022 18:28
@chanceaclark chanceaclark marked this pull request as ready for review September 20, 2022 18:33
@chanceaclark chanceaclark requested review from a team as code owners September 20, 2022 18:33
Copy link
Contributor

@jorgemoya jorgemoya left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! 👍

@@ -61,7 +63,7 @@ function transformCode(input: string): string {
}

export interface CodePreviewProps {
children?: React.ReactNode;
children?: CodePreviewChildren;
Copy link
Contributor

Choose a reason for hiding this comment

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

👏

Comment on lines +15 to 25
renderHook(() => useUpdateItems(), {
wrapper: class Wrapper extends Component<PropsWithChildren<unknown>> {
override componentDidCatch(err: unknown) {
error = err;
}
override render() {
return this.props.children;
}
},
});

Copy link
Contributor

Choose a reason for hiding this comment

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

😵‍💫

Comment on lines +21 to +25
Auto dismiss after 5 seconds.
{/* I hate using a br but the as prop doesn't support div for now */}
{/* TODO: Support div for as prop */}
<br />
<Small as="span">Note: Only valid when used with AlertManager.</Small>
Copy link
Contributor

Choose a reason for hiding this comment

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

:cringe:

Copy link
Contributor

@bc-juanvasquez bc-juanvasquez left a comment

Choose a reason for hiding this comment

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

💭

@chanceaclark chanceaclark added the do not merge Don't merge this PR! label Sep 26, 2022
@chanceaclark
Copy link
Contributor Author

Going to wait till #985 is released before releasing this PR.

@chanceaclark chanceaclark removed the do not merge Don't merge this PR! label Sep 27, 2022
Copy link
Contributor

@MariaJose MariaJose left a comment

Choose a reason for hiding this comment

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

🥳

@chanceaclark chanceaclark merged commit 51c688b into bigcommerce:master Sep 27, 2022
@chanceaclark chanceaclark deleted the feat/react-18 branch September 27, 2022 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants