LG-11361: Extract Document Capture Step Code#9433
Conversation
| // getReviewStep needs to be called even if we're not going to use it because it contains | ||
| // a hook (in a context). Conditionally calling a hook causes problems in React, so | ||
| // always calling getReviewStep is necessary | ||
| const reviewStep: FormStep = useReviewStep(submissionError); |
There was a problem hiding this comment.
Like components, I'd expect a "one hook per file" rule. What's the downside of just assigning the value generated in useReviewStep inline here, aside from noise? Or better yet, we can assign it further down the file past the conditional early return (i.e. alongside locationStep and inPersonProofingSteps).
| }, | ||
| ].filter(Boolean) as FormStep[]); | ||
|
|
||
| const inPersonProofingStepNames = ['location', 'prepare', 'switch_back']; |
There was a problem hiding this comment.
Since this value never changes, I'd suggest moving it somewhere toward the top of the file to avoid allocating a new array every time the component is rendered.
const IN_PERSON_PROOFING_STEP_NAMES = ['location', 'prepare', 'switch_back'];Less important (especially since this code already existed), but we could also consider using a Set for this, which is a little more accurate in representing a set of unique values, and has some performance benefit (not that I expect performance to be much an issue here).
const IN_PERSON_PROOFING_STEP_NAMES = new Set(['location', 'prepare', 'switch_back']);| }; | ||
| }; | ||
|
|
||
| export const useSteps = (submissionError: Error | undefined) => { |
There was a problem hiding this comment.
Can we add a return type here? My instinct from the previous code is that this should be returning FormStep[], but looking at the logic of this function I think we may also be returning undefined in some cases. Is that expected?
| export const useSteps = (submissionError: Error | undefined) => { | |
| export const useSteps = (submissionError: Error | undefined): FormStep[] => { |
|
Everything you pulled out is working as I'd expect when testing. Doing this work now seems like a good idea rather than when you are trying to add/move steps. While testing I am seeing behavior that surprised me. I uploaded images to cause a barcode error. When I click Continue I am taken to the upload page with a large error. Add new photos takes me to the same upload page without the error banner. I tested main. This behavior is not isolated to your branch, I just happen to notice it. Bug? 
|
|
Closing for now. |
3 participants
🎫 Ticket
https://cm-jira.usa.gov/browse/LG-11361
🛠 Summary of changes
This is a code cleanup ticket. In anticipation that we may need to add/remove steps from the document capture javascript, it extracts and simplifies the code that figures out what those steps should be in various situations.
🥅 Goals 🥅
I'm doing this for two reasons