-
Notifications
You must be signed in to change notification settings - Fork 1
[MPDX-9267] Improve approval process card #1592
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
|
Preview branch generated at https://9267-approval-process-card.d3dytjb8adxkk5.amplifyapp.com |
Bundle sizes [mpdx-react]Compared against d51f585 No significant changes found |
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.
This looks great. I need to add something very similar for ASR, thanks for paving the way!
| overCapName, | ||
| overCapSalary, | ||
| } = useCaps(); | ||
| const overCombinedCap = |
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.
Question: Do you think it makes sense to change this variable name now that it isn't calculating combined cap?
| sx={{ fontWeight: 'bold' }} | ||
| > | ||
| {t('Additional Information')} | ||
| {tier === ProgressiveApprovalTierEnum.DivisionHead |
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 noticed this line a few times in this file, could we just assign it a variable?
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.
Should we add tests for other division tiers now?
Description
MPDX-9267
Testing
Checklist:
/pr-reviewcommand locally and fixed any relevant suggestions