-
Notifications
You must be signed in to change notification settings - Fork 4
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
1577 - Upgrade frontend deps #3327
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3327 +/- ##
===========================================
- Coverage 91.48% 91.45% -0.03%
===========================================
Files 299 299
Lines 8595 8605 +10
Branches 636 638 +2
===========================================
+ Hits 7863 7870 +7
- Misses 615 617 +2
- Partials 117 118 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
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.
I want to spend more time with this and test across a few different devices/OSes but for the most part this is already looking pretty good!
Automated scans I've run so far are showing up clear and I've only turned up a handful of issues (mix of a11y and general QA) with initial manual testing:
I'll confirm for sure in a subsequent review but it looks like this has fixed #2307 as expected.
thank you @reitermb! i've reviewed your findings and made three tickets. The
|
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.
I didn't find any other issues. Everything that I tried worked like a charm and didn't look weird! Great work on this!
section, | ||
}, | ||
}) | ||
} else { |
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.
Love how much nicer this all is now!
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.
all good!
@@ -1,3 +1,6 @@ | |||
@use "uswds-core" as *; |
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.
Thanks @jtimpe! Just to make sure it's here too I'm good signing off of the padding fix. Just to track some of the more complicated followup a11y testing, in terms of next steps is the plan to merge this or keep it open until the spinoff fixes are added? |
Thanks @reitermb! And that's a good question. Perhaps @andrew-jameson or @ADPennington could weigh in. My assumption was that we'd merge/close this and work the follow-on tickets based on priority. |
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.
Thanks @jtimpe! Just to make sure it's here too I'm good signing off of the padding fix. Just to track some of the more complicated followup a11y testing, in terms of next steps is the plan to merge this or keep it open until the spinoff fixes are added?
Thanks @reitermb! And that's a good question. Perhaps @andrew-jameson or @ADPennington could weigh in. My assumption was that we'd merge/close this and work the follow-on tickets based on priority.
I tested the userflows for acf and non-acf users for sign-in, request access, and data submission, and viewing error reports. LGTM 🚀 This can be merged. In terms of priority of the follow-on tickets:
- Resolve vulnerabilities in frontend dependencies after React upgrade #3328 (security fix)
- Fix GovBanner component behavior after React upgrade #3353 (distracting user experience)
- Fix Data File search results not loading on first click #3354 (pre-existing bug)
- Fix accessibility issue with search results header in Reports page #3352 (pre-existing bug)
@reitermb please schedule a time next week with @ttran-hub for pair- a11y testing and review a11y-related tickets coming out of this PR. This will not block approval/merge.
Summary of Changes
Pull request closes #1577
The goal of this was primarily to get
react-scripts
updated to version 5. I took the opportunity to bump all the dependencies to the latest possible (downgrading a select few only to avoid conflicts). Along the way, the following tasks were also necessaryfile-type
library (requires node runtime, no longer compatible) withfile-type-checker
(browser runtime)A tech memo was drafted covering the upgrade process, as well as errors and resolutions experienced during the dependency upgrade.
The following tickets were created as follow-on work
fetch
to replaceaxios
- Replace axios with native Fetch API for HTTP requests #3333src/assets/uswds/_uswds-theme-general
- Remove duplicate default values from USWDS theme file #3335How to Test
Deliverables
More details on how deliverables herein are assessed included here.
Deliverable 1: Accepted Features
Checklist of ACs:
updated to latest major versionreplaced with a browser-runtime-compatible alternativelfrohlich
and/oradpennington
confirmed that ACs are met.Deliverable 2: Tested Code
CodeCov Report
comment in PR)CodeCov Report
comment in PR)Deliverable 3: Properly Styled Code
Deliverable 4: Accessible
iamjolly
andttran-hub
using Accessibility Insights reveal any errors introduced in this PR?@ttran-hub is OOO, so I recommend pair-testing with @reitermb upon return.
Deliverable 5: Deployed
Deliverable 6: Documented
Deliverable 7: Secure
follow-on ticket Resolve vulnerabilities in frontend dependencies after React upgrade #3328
Deliverable 8: User Research
Research product(s) clearly articulate(s):