Jmax/lg 9794 delete front end async document capture code#8396
Jmax/lg 9794 delete front end async document capture code#8396
Conversation
jmax-gsa
force-pushed
the
jmax/LG-9794-delete-front-end-async-document-capture-code
branch
2 times, most recently
from
920e48e to
aeec0ba
Compare
app/javascript/packages/document-capture/components/document-capture.tsx
Outdated
Show resolved
Hide resolved
jmax-gsa
force-pushed
the
jmax/LG-9794-delete-front-end-async-document-capture-code
branch
from
5ef048f to
b05f9b2
Compare
jmax-gsa
force-pushed
the
jmax/LG-9794-delete-front-end-async-document-capture-code
branch
from
328eac1 to
28862ba
Compare
aduth
left a comment
aduth
left a comment
There was a problem hiding this comment.
I left a few initial comments, but I'll plan to take a closer look at this in my morning.
app/javascript/packages/document-capture/components/document-capture.tsx
Show resolved
Hide resolved
app/javascript/packages/document-capture/components/document-capture.tsx
Outdated
Show resolved
Hide resolved
spec/javascript/packages/document-capture/components/review-issues-step-spec.jsx
Outdated
Show resolved
Hide resolved
| import { useContext } from 'react'; | ||
| import { t } from '@18f/identity-i18n'; | ||
| import { FormError } from '@18f/identity-form-steps'; | ||
| import { trackError } from '@18f/identity-analytics'; |
There was a problem hiding this comment.
This was our only use of trackError, so we could remove it from analytics/index.ts if we wanted.
There was a problem hiding this comment.
I'll remove it if that's the team consensus, but I think it should stay for future use.
There was a problem hiding this comment.
I fall more on the side of removing anything that's unused and that can be brought back later, but I don't feel too strongly in this case, since it's honestly surprising this is the only error we log on the frontend, and seems like something we'd want to have easy access to.
Removed withProps for ReviewIssuesStep; changing the type works better.
aduth
left a comment
aduth
left a comment
There was a problem hiding this comment.
There are some conflicts on the branch and left a comment about package.json changes, but otherwise LGTM 👍
There was a problem hiding this comment.
Were the changes in this file intended to be committed? I'd prefer to keep the Mocha colors at least, and would recommend configuration go in .mocharc.js and tsconfig.json so that it applies to single-file test runs (using yarn mocha directly).
There was a problem hiding this comment.
Eeep. Thank you; those should not have been committed.
| import { useContext } from 'react'; | ||
| import { t } from '@18f/identity-i18n'; | ||
| import { FormError } from '@18f/identity-form-steps'; | ||
| import { trackError } from '@18f/identity-analytics'; |
There was a problem hiding this comment.
I fall more on the side of removing anything that's unused and that can be brought back later, but I don't feel too strongly in this case, since it's honestly surprising this is the only error we log on the frontend, and seems like something we'd want to have easy access to.
jmax-gsa
deleted the
jmax/LG-9794-delete-front-end-async-document-capture-code
branch
4 participants
No description provided.