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

Upgrade to React 18 #2847

Merged
merged 4 commits into from
Feb 1, 2024
Merged

Upgrade to React 18 #2847

merged 4 commits into from
Feb 1, 2024

Conversation

Juneezee
Copy link
Contributor

@Juneezee Juneezee commented Feb 1, 2024

Description and Motivation

Closes #1728.

Has this been tested? How?

Screenshots (if appropriate)

Types of changes

  • Refactor / chore

New frontend preview link is below in the Netlify comment 😎

Closes #1728.

Signed-off-by: Eng Zer Jun <[email protected]>
Copy link

netlify bot commented Feb 1, 2024

Deploy Preview for health-equity-tracker ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit b103cbf
🔍 Latest deploy log https://app.netlify.com/sites/health-equity-tracker/deploys/65bbe1b3155a83000819620e
😎 Deploy Preview https://deploy-preview-2847--health-equity-tracker.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Comment on lines +8 to +11
"react-lazyload": {
"react": "^18.2.0",
"react-dom": "^18.2.0"
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

nice. thanks for the helpful comments!

@@ -17,7 +21,6 @@
"@mui/icons-material": "^5.15.6",
"@mui/lab": "^5.0.0-alpha.124",
"@mui/material": "^5.14.9",
"@mui/styles": "^5.14.9",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

npm ERR! While resolving: @mui/[email protected]
npm ERR! Found: [email protected]
npm ERR! node_modules/react
npm ERR!   react@"^18.2.0" from the root project
npm ERR!   peer react@">=16.8.0" from @emotion/[email protected]
npm ERR!   node_modules/@emotion/react
npm ERR!     @emotion/react@"^11.11.1" from the root project
npm ERR!     peer @emotion/react@"^11.0.0-rc.0" from @emotion/[email protected]
npm ERR!     node_modules/@emotion/styled
npm ERR!       @emotion/styled@"^11.11.0" from the root project
npm ERR!       5 more (@mui/lab, @mui/material, @mui/styled-engine, ...)
npm ERR!     5 more (@mui/lab, @mui/material, @mui/styled-engine, ...)
npm ERR!   33 more (@emotion/styled, ...)

mui/material-ui#32142. We are not using @mui/styles anywhere so this can be removed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

good catch!

Comment on lines -55 to +58
"use-react-screenshot": "^3.0.0",
"use-react-screenshot": "^4.0.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

npm WARN Could not resolve dependency:
npm WARN peer react@"^17.0.2" from [email protected]
npm WARN node_modules/use-react-screenshot
npm WARN   use-react-screenshot@"^3.0.0" from the root project
npm WARN ERESOLVE overriding peer dependency
npm WARN While resolving: [email protected]
npm WARN Found: [email protected]
npm WARN node_modules/react-dom
npm WARN   react-dom@"^18.2.0" from the root project

No changelog provided by the author. Looking at https://github.com/vre2h/use-react-screenshot/commits/master/, there seems to be no breaking changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@eriwarr @kccrtv we should see if this fixes #2186 ?

{error && (
{isError && (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

src/pages/News/SinglePost.tsx:143:13 - error TS2322: Type 'unknown' is not assignable to type 'ReactNode'.

143             {error && (
                ~~~~~~~~~~~
144               <img
    ~~~~~~~~~~~~~~~~~~
... 
150               />
    ~~~~~~~~~~~~~~~~
151             )}
    ~~~~~~~~~~~~~~

Comment on lines -28 to +38
ReactDOM.render(
<StrictMode>
<QueryClientProvider client={queryClient}>
<App />
</QueryClientProvider>
</StrictMode>,
document.getElementById('root')
)
const container = document.getElementById('root')
if (container !== null) {
const root = createRoot(container)
root.render(
<StrictMode>
<QueryClientProvider client={queryClient}>
<App />
</QueryClientProvider>
</StrictMode>
)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Juneezee
Copy link
Contributor Author

Juneezee commented Feb 1, 2024

The CI doesn't show which exact test is causing the timeout. I ran the E2E locally and got the error:

Error: expect(locator).toBeVisible()

Locator: getByRole('heading', { name: 'Start Your Search', exact: true })
Expected: visible
Received: hidden
Call log:
  - expect.toBeVisible with timeout 90000ms
  - waiting for getByRole('heading', { name: 'Start Your Search', exact: true })

await expect(page.getByRole('heading', { name: 'Start Your Search', exact: true })).toBeVisible();

Video by Playwright:

7577eeb21aff7e4d9304b9fe4ada59fac7829e28.webm

For some unknown reasons, the last step is being shown first instead of the first step, https://deploy-preview-2847--health-equity-tracker.netlify.app/exploredata?mls=1.covid-3.00&mlp=disparity&dt1=covid_cases&onboard=true. I'm looking into this now

Our <Onboarding /> components gets rendered before <MadlibUI /> finishes
parsing the query params and selects the "COVID-19" dropdown. This is
why the ".covid-dropdown-topic" CSS class is not available initially and
gets skipped.

Reference: #2847 (comment)
Signed-off-by: Eng Zer Jun <[email protected]>
'.covid-dropdown-topic',
'#madlib-box',
Copy link
Contributor Author

@Juneezee Juneezee Feb 1, 2024

Choose a reason for hiding this comment

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

The <Onboarding /> components gets rendered before <MadlibUI /> finishes parsing the query params and selects the "COVID-19" dropdown. This is why the .covid-dropdown-topic CSS class is not available initially and gets skipped to the final step.

The madlib-box ID is more consistent because it is already available when the page is rendered.

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

wow, great work debugging the issue. we really appreciate your help!

@benhammondmusic benhammondmusic merged commit 9fa507a into SatcherInstitute:main Feb 1, 2024
10 checks passed
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.

Should Upgrade to React 18
2 participants