-
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
3407 - FRA Reports - submission history #3460
base: 3398-fra-frontend
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 3398-fra-frontend #3460 +/- ##
=====================================================
- Coverage 90.80% 90.63% -0.17%
=====================================================
Files 318 318
Lines 9204 9251 +47
Branches 721 733 +12
=====================================================
+ Hits 8358 8385 +27
- Misses 710 727 +17
- Partials 136 139 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
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.
Works great for me locally
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 -- just need to make sure the required fields have an * next to them like the TANF Data Files
@@ -11,13 +11,20 @@ import { accountCanSelectStt } from '../../selectors/auth' | |||
import { handlePreview, tryGetUTF8EncodedFile } from '../FileUpload/utils' |
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.
Another interesting visual bug. Upload and submit a file, click to change the file, cancel the file selector modal, see that the original file and download button are still there. This is different behavior than the data files page.
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 was also able to get the file input element to collapse like the datafiles one. The PR that fixed the issue for the datafiles page merged so you should be able to use the same logic. Note I had to actually expose functions to handle that, it wasn't the removal of our custom css classes completely causing the issue.
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 callouts, thank you! i will take a look
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 was only able to address your first comment here - hiding the download button when there's a previous upload and an error.
For the second and third comments, i did not experience the same behavior locally. The FRA and DF pages appear to work the same with respect to the original file/download button, and i actually have the input collapse every time i select a file when trying to implement the remove preview logic from the other PR. happy to pair and try to work through it if you're still having issues
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.
Everything looks good on this. I also could not reproduce the second bug I called out. As for the file collapse, Im going to write up a bug ticket for it. The preview logic is a little different than the datafiles page and can't use the exact same solution to prevent the input collapse.
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 TM
Summary of Changes
Pull request closes #3407
How to Test
fra_access
feature flag for your userDeliverables
More details on how deliverables herein are assessed included here.
Deliverable 1: Accepted Features
Checklist of ACs:
lfrohlich
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?Deliverable 5: Deployed
Deliverable 6: Documented
Deliverable 7: Secure
Deliverable 8: User Research
Research product(s) clearly articulate(s):