-
Notifications
You must be signed in to change notification settings - Fork 1
[No-Jira] Added steps to percent calculation in ASR #1549
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
base: main
Are you sure you want to change the base?
Conversation
Bundle sizes [mpdx-react]Compared against 4f67f48 No significant changes found |
|
Preview branch generated at https://update-additional-salary-percentage.d3dytjb8adxkk5.amplifyapp.com |
|
I really like the combination of steps and fields filled. I think this is a good middle ground for ASR. @canac what do you think? |
wjames111
left a comment
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 great thanks for updating this! I think this is a good in between, that being said Caleb brought up a good point that it is likely most of these fields will go unitized. In light of this factoring in the fields might be a little misleading. I'm thinking maybe we should just go with calculating the steps only.
| (value) => typeof value === 'string' || typeof value === 'number', | ||
| ); | ||
|
|
||
| const stepsCompleted = currentIndex + 1; |
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.
This makes the form start at 5%. Although changing this makes the form end at 95%. I would think we want it to start at 0% and end at 100%
| const stepsCompleted = currentIndex + 1; | |
| const stepsCompleted = currentIndex; |
|
I think it should be 0% on the first page and 100% on the confirmation page. But I don't have strong opinions about what the percentage shows on the actual form. Honestly, I'm not sure that the progress indicator adds much value on the ASR form, but I'm also not opposed to keeping it. Sorry, that's probably not super helpful, but I don't think it matters too much. |
|
@canac Katelyn and I went back and forth on this. I'm unsure what the best way is as well, but fine with what Katelyn has done for now and it's easy to change it later on. If we started at 0% If we started at 25% So it really comes down to where you want the 25% jump, at the start or the beginning. |
|
Actually, maybe I'm thinking of MHA |
|
@dr-bizz this one's weird as it only really has 1 true step, there's the about the form, then the complete the form and the receipt. Initially we thought it made the most sense to go by fields filled out for the percentage but then realized, for most people the majority of fields would stay blank. This PR combines both the steps and the fields filled out to get the percentage. I think we probably want to change it so that either it's only steps being calculated or do away entirely with the percent. |
Description
Since the percentage in ASR is calculated based on number of filled fields, it may be confusing to users when the first step shows 0% and does not increase after clicking "Continue". To eliminate this confusion, I included the steps in the percentage calculation so that progress is reflected immediately. This logic aligns with the other reports, which calculate progress based on step completion.
Testing
/reports/additionalSalaryRequestChecklist:
/pr-reviewcommand locally and fixed any relevant suggestions