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

Tforkner/4376 upgrades #4498

Merged
merged 91 commits into from
Feb 1, 2023
Merged

Tforkner/4376 upgrades #4498

merged 91 commits into from
Feb 1, 2023

Conversation

thetif
Copy link
Contributor

@thetif thetif commented Jan 26, 2023

Resolves #4376

Description

Upgraded node from 16.16.0 to 16.19.0. Upgraded babel, eslint, and all of the patch updates.

This can't be tested in Preview until Bill updates the AMI.

Significant changes or possible side effects

I had to make some changes to the APD New page because the Create New APD button wasn't disabling every time it needed to.

Also the update to core-axe came with a new rule. You aren't allowed to have an empty . In the Quarterly Federal Share and Estimated Quarterly Incentive Payments tables, were using aria-hidden on the first header cell and axe was flagging that. I took off the aria-hidden and it's passing now.

This pull request is ready to code review when

  • Automated tests are updated (and all tests are passing)
  • Pull request has been labeled, if applicable with feature, content, bug,
    tests, refactor

This pull request is ready to test when

  • Code has been reviewed by someone other than the original author

This pull request is ready to review when the QA has

  • Verified the functionality related to the change
  • Verified that the change works with Narrator on Windows
  • Verified that the change works with VoiceOver on Mac
  • Verified all updated pages with the WAVE tool
  • Verified tab and keyboard navigation functionality

This pull request can be merged when

  • Design has approved the experience
  • Product has approved the experience

@cms-eapd-bot
Copy link

cms-eapd-bot commented Jan 27, 2023

See this pull request in action: https://ec2-18-233-210-18.compute-1.amazonaws.com

edd22a2

@thetif thetif added the dependencies Pull requests that update a dependency file label Jan 27, 2023
@thetif thetif marked this pull request as ready for review January 27, 2023 20:58
@thetif thetif requested a review from tbolt January 27, 2023 20:58
Copy link
Contributor

@tbolt tbolt left a comment

Choose a reason for hiding this comment

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

Looks good

Copy link
Contributor

@amyd11 amyd11 left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for adding in all those id's throughout the code.

Question (non-blocking): When creating an APD, the name is not a required field- should that be required as well?

@@ -57,14 +54,15 @@ const ApdNew = ({ createApd: create }) => {
asNeededUpdate: false
};

const yearOptions = [thisFFY, thisFFY + 1, thisFFY + 2].map(y => `${y}`);
const yearOptions = defaultAPDYearOptions();
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice clean up!

Copy link
Contributor

@Sun-Mountain Sun-Mountain left a comment

Choose a reason for hiding this comment

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

🚀

Copy link
Contributor

@mirano-darren mirano-darren left a comment

Choose a reason for hiding this comment

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

LGTM! Bug ticket with create new APD page documented in #4502

Copy link
Contributor

@jeromeleecms jeromeleecms left a comment

Choose a reason for hiding this comment

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

Good pick up on the APD switching bug (but that was already there). Looks good to move forward - also approved in Chromatic.

@thetif thetif merged commit 1de79e3 into main Feb 1, 2023
@thetif thetif deleted the tforkner/4376-upgrades branch February 1, 2023 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Maintenance] Update node
9 participants