-
Notifications
You must be signed in to change notification settings - Fork 30
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
Tforkner/4376 upgrades #4498
Conversation
…sue with __dirname
…3698-remove-babel
See this pull request in action: https://ec2-18-233-210-18.compute-1.amazonaws.com |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
…ents and QuarterlyBudgetSummary
There was a problem hiding this 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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice clean up!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
There was a problem hiding this 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
There was a problem hiding this 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.
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
tests, refactor
This pull request is ready to test when
This pull request is ready to review when the QA has
This pull request can be merged when