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: update major dependencies #1589

Merged
merged 18 commits into from
Apr 20, 2023
Merged

feat: update major dependencies #1589

merged 18 commits into from
Apr 20, 2023

Conversation

ludtkemorgan
Copy link
Collaborator

@ludtkemorgan ludtkemorgan commented Mar 17, 2023

This PR addresses #1591

  • This change addresses the issue in full
  • This change addresses only certain aspects of the issue
  • This change is a dependency for another issue
  • This change has a dependency from another issue

Description

This upgrades several dependencies that have already been upgraded in core this includes:

  • Node from 14 to 18
  • Next to 12
  • React to 18
  • Cypress from 9 to 12

It also accomplishes the following:

  • removes storybook from detroit-ui-components
  • update all test ids to data-testid

How Can This Be Tested/Reviewed?

This touches every page so it is recommended to go to every page and do a quick check. These changes are already in and have been tested so we don't have to be as thorough, but there are some changes that are unique so a quick passover is recommended.

You will need to test locally since it has to have a new deployed version of the backend

Checklist:

  • My code follows the style guidelines of this project
  • I have added QA notes to the issue with applicable URLs
  • I have performed a self-review of my own code
  • I have reviewed the changes in a desktop view
  • I have reviewed the changes in a mobile view
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have assigned reviewers
  • I have run yarn generate:client and/or created a migration if I made backend changes that require them
  • I have exported any new pieces added to ui-components
  • My commit message(s) is/are polished, and any breaking changes are indicated in the message and are well-described
  • Commits made across packages purposefully have the same commit message/version change, else are separated into different commits

Reviewer Notes:

Steps to review a PR:

  • Read and understand the issue, and ensure the author has added QA notes
  • Review the code itself from a style point of view
  • Pull the changes down locally and test that the acceptance criteria is met
  • Also review the acceptance criteria on the Netlify deploy preview (noting that these do not yet include any backend changes made in the PR)
  • Either explicitly ask a clarifying question, request changes, or approve the PR if there are small remaining changes but the PR is otherwise good to go

On Merge:

If you have one commit and message, squash. If you need each message to be applied, rebase and merge.

@netlify
Copy link

netlify bot commented Mar 17, 2023

Deploy Preview for detroit-storybook-dev failed.

Name Link
🔨 Latest commit 10eab3d
🔍 Latest deploy log https://app.netlify.com/sites/detroit-storybook-dev/deploys/6430827c2d8a340008d238a4

@netlify
Copy link

netlify bot commented Mar 17, 2023

Deploy Preview for detroit-public-dev ready!

Name Link
🔨 Latest commit 22581b2
🔍 Latest deploy log https://app.netlify.com/sites/detroit-public-dev/deploys/64414c09a386fd0008287825
😎 Deploy Preview https://deploy-preview-1589--detroit-public-dev.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 settings.

@netlify
Copy link

netlify bot commented Mar 17, 2023

Deploy Preview for detroit-partners-dev ready!

Name Link
🔨 Latest commit 22581b2
🔍 Latest deploy log https://app.netlify.com/sites/detroit-partners-dev/deploys/64414c09aeab2c0008c578dc
😎 Deploy Preview https://deploy-preview-1589--detroit-partners-dev.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 settings.

@ludtkemorgan ludtkemorgan force-pushed the 3317/node branch 6 times, most recently from 1a9bdd6 to 10eab3d Compare April 7, 2023 20:52
@@ -74,6 +74,12 @@ const UnitsSummaryAmiForm = ({
[amiInfo, setAmiPercentageOptions]
)

useEffect(() => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is an edge case only appearing on the cypress tests where the amiChartId is not being updated before the onChange event happens. By moving it to the useEffect we can guarantee that the order will happen correctly

@ludtkemorgan ludtkemorgan changed the title 3317/node feat: update major dependencies Apr 12, 2023
@ludtkemorgan ludtkemorgan marked this pull request as ready for review April 12, 2023 18:58
@@ -0,0 +1,87 @@
import React from "react"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This file and the ones below it were accidentally missed when pulling over the refactoring commits from core. Specifically where we pulled things out of ui-components and into the corresponding packages they are used in

@emilyjablonski
Copy link
Collaborator

Screenshot 2023-04-14 at 4 39 32 PM
Looking great! This is the only issue I've seen so far in that right hand panel on the housings basics page.

ludtkemorgan and others added 14 commits April 17, 2023 13:29
* feat: upgrade to node 19 and next 12

* fix: add netlify plugin

* fix: add netlify plugin partners

* fix: remove netlify plugins

* fix: add netlify plugin to toml

* Revert "fix: remove netlify plugins"

This reverts commit 840b4a1.

* fix: switch to node 16

* fix: update all places referencing node

* Revert "fix: update all places referencing node"

This reverts commit 9f13a5a.

* Revert "fix: switch to node 16"

This reverts commit 8acbfbe.

* fix: re-installed with 18
@ludtkemorgan
Copy link
Collaborator Author

Screenshot 2023-04-14 at 4 39 32 PM Looking great! This is the only issue I've seen so far in that right hand panel on the housings basics page.

Nice catch, there were also a couple of places where the markdown wasn't getting formatted. I've pushed up an update (remove new line below the RenderIf)

Copy link
Collaborator

@emilyjablonski emilyjablonski left a comment

Choose a reason for hiding this comment

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

LGTM! Down to bash next week if we have time 🔌

@ColinBuyck
Copy link
Collaborator

Little thing but noticing that the "page not found" page is different. I definitely like the PR version better but just want to flag to make sure that its not pointing to any other issues
Screenshot 2023-04-18 at 4 52 52 PM

@ColinBuyck
Copy link
Collaborator

Also noticing when trying to add a new listing, I get an alert to resolve errors but don't see anything marked. Additionally, I get the following error in the console.
Screenshot 2023-04-18 at 5 19 33 PM

@emilyjablonski
Copy link
Collaborator

^ spooky to me that cypress didn't see this

@ColinBuyck
Copy link
Collaborator

Good point 👻 @emilyjablonski I revisited this after deleting node modules and reinstalling everything, and can't seem to reproduce the failed listing publish from before but without fail, the console errors show just from opening the add listing page.

@ludtkemorgan
Copy link
Collaborator Author

@ColinBuyck That console error is happening core as well. I created a ticket for it recently. bloom-housing#3392

@ludtkemorgan
Copy link
Collaborator Author

Little thing but noticing that the "page not found" page is different. I definitely like the PR version better but just want to flag to make sure that its not pointing to any other issues Screenshot 2023-04-18 at 4 52 52 PM

Nice catch. This was because of a new error page that I added. I've updated it to the following. It's different than what is in prod, but I think this is what was originally intended
image

Copy link
Collaborator

@ColinBuyck ColinBuyck left a comment

Choose a reason for hiding this comment

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

LGTM assuming Cypress test are just being being flakyyy 🫓

@ludtkemorgan ludtkemorgan merged commit 8712e8c into dev Apr 20, 2023
@ludtkemorgan ludtkemorgan deleted the 3317/node branch April 20, 2023 15:13
ludtkemorgan added a commit that referenced this pull request Jul 24, 2023
ludtkemorgan added a commit that referenced this pull request Jul 26, 2023
* feat: update major dependencies (#1589)

* fix: update bull version (#1620)

* 1577/download button can show and be non functional (#1621)

* fix: display paper application button download when application is present

* fix: hide redundant sections when data not provided

* refactor: use paperMethod prop to decide if display download button

* feat: uptake uic components (#1586)

* fix: change reset to setValue for disableUnitsAccordion (#1628)

* fix: various arabic layout fixes (#1629)

* fix: run correct node version

* fix: remove missed detroit-ui-components

* fix: prettier fixes

---------

Co-authored-by: Krzysztof Zięcina <[email protected]>
Co-authored-by: Emily Jablonski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants