-
Notifications
You must be signed in to change notification settings - Fork 123
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
VACMS-18974 DUW v2 flow update #31562
Conversation
@chriskim2311, could you make sure everything in the new form matches the mural before we continue reviewing and if there are deviations for specific reasons we call these out? I think I'm specifically looking at the edit logic. So far I've found at least one scenario that doesn't look to match the mural:
|
@laflannery Thanks for the call out! The questions that branch are the following: Discharge Year, Reason, Previous Application, Previous Application Type, Previous Application Year. There are scenarios where the Previous Application Type answer can change the flow to Prior Service or Failure to Exhaust. |
This was just confusion/misunderstanding on my part. Chris explained it to me in slack |
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. Tested on the flow mentioned. Back and continue seem to retain information and flow seems correct re:Mural
@chriskim2311 @thejordanwood and I ended up reviewing together because it was helpful, we have a few additional notes:
Let me know if there are any technical limitations to the issues we are seeing with the buttons. I do know this is complex and if there needs to be a discussion about a different solution let us know. |
@chriskim2311 I'm going to update the Mural tomorrow to reflect this change if this is something you can do. |
@laflannery @thejordanwood Thanks for reviewing! I have some comments below.
I have added this to all questions when a user clicks edit, if Amanda does not approve it I can revert the change.
I did make some changes for this to work as well.
Completing the wizard then editing and clicking back to the home screen do we not want to save any answers? Also from my understanding we want to only show empty questions in the new flow and skip any answered questions. This was based on Dave's original logic.
I did find a bug that I believed resolved this issue. If you hit a scenario where the buttons aren't working can you open the console and send me a screenshot, there should be an error shown, so I can debug further. |
@chriskim2311 I'm not done with functionality testing but I needed a break so I switched to accessibility testing. Only one note:
|
@chriskim2311 I've updated the Mural to show:
Let me know if something is unclear! |
@laflannery Thanks I have updated the alert mark up. |
@chriskim2311 Jordan and I put the edge cases we were finding in this google doc because it was easier to format and make sense than in a comment. Can you take a look and then let the group know if you feel these are highlighting larger issues and we maybe should go with a simpler option as MVP or if we might be able to sort this out, thanks! Word doc for other folks: |
(From Slack DMs) I reviewed against Laura's doc. I used the answers from Laura's screenshot on #2 and saw that the red text is still happening. I saw that she mentioned Jordan couldn't reproduce but I tested twice with those answers and it happened both times. Random thing I noticed: while clicking Edit on the review screen, you can see two focus boxes (yellow squares) Question: should the review screen show the Your information was updated info box if the answer wasn't changed when editing? My suspicion is yes (to keep logic simpler) but I wanted to double check. I didn't see any other issues; logic looks good! |
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.
Chris and I discussed #2 (above) in DMs; this is intentional behavior. All is good! Great work, Chris
Going to add a |
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.
Some non-blocking comments for consideration
@@ -74,6 +77,9 @@ const Dropdown = ({ | |||
|
|||
if (forkableQuestions.includes(shortName) && editMode) { | |||
toggleQuestionsFlowChanged(true); | |||
toggleAnswerChanged(true); |
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.
Consider: I've noticed in a few files in this PR that we're duplicating toggleAnswerChanged(true)
on both sides of an if
statement. Is this for readability / clarity? Perhaps we could consolidate and place it once above the if
statement.
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! I'll clean this up it needs to be in the if statement only. But I can rewrite it to only write it once.
@@ -93,31 +93,53 @@ export const navigateForward = ( | |||
!formResponses[nextShortName] && | |||
editMode | |||
) { | |||
updateRouteMap([...routeMap, ROUTES?.[nextShortName]]); | |||
if (routeMap[routeMap.length - 1] !== ROUTES?.[nextShortName]) { |
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 think we could benefit from a comment(s) explaining what's going on here briefly, and/or smaller utility functions that navigateForward
uses to help keep things more readable.
pushToRoute(nextShortName, router); | ||
return; | ||
} | ||
|
||
// This handles the edge case of the edit question not being skipped |
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.
Consider: don't be afraid of longer variable names or more detailed comments to explain complex behavior. For instance, "edit question" may not be clear to a stranger to this code; maybe "question being edited." (Same with variable editQuestion
; this could be questionSelectedToEdit
) Not sure if my suggestions make it more clear; just offering some perspective
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.
That makes sense I can make these changes
Approved...finally! |
Approved! |
Are you removing, renaming or moving a folder in this PR?
Did you change site-wide styles, platform utilities or other infrastructure?
Summary
This PR updates the flow and button functionality of DUW v2. We made changes to the continue/back and edit logic throughout the entire flow.
Branching Questions: Discharge Year, Reason, Previous Application, Previous Application Type, Previous Application Year
Related issue(s)
department-of-veterans-affairs/va.gov-cms#18974
Testing done
Tested flows and all AC functionality throughout the flow and the review page.
Testing URL:
/discharge-upgrade-instructions/introduction/
Mural for testing: https://app.mural.co/t/departmentofveteransaffairs9999/m/departmentofveteransaffairs9999/1696352911500/80253b1c7200212cbcbcfb940d2cf4a29fed8417?sender=ua4eca0cc168eff677bd03940
Screenshots
What areas of the site does it impact?
DUW v2
Acceptance criteria
Quality Assurance & Testing
Error Handling