-
Notifications
You must be signed in to change notification settings - Fork 1
MPDX-9130 Combine MHA Pages #1547
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
|
|
Preview branch generated at https://mpdx-9130-combine-edit-new-pages.d3dytjb8adxkk5.amplifyapp.com |
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.
Great work on this! Works really well, I had a few small comments and some suggestions but other then that it looks really good.
| const modeLabel = | ||
| type === PageEnum.New ? 'New' : type === PageEnum.Edit ? 'Edit' : 'View'; | ||
| const title = t("{{mode}} Minister's Housing Allowance Request", { | ||
| mode: | ||
| type === PageEnum.New ? 'New' : type === PageEnum.Edit ? 'Edit' : 'View', | ||
| mode: modeLabel, |
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 would update the enum to 'New', 'Edit' and 'View' then just use type here.
| const modeLabel = | |
| type === PageEnum.New ? 'New' : type === PageEnum.Edit ? 'Edit' : 'View'; | |
| const title = t("{{mode}} Minister's Housing Allowance Request", { | |
| mode: | |
| type === PageEnum.New ? 'New' : type === PageEnum.Edit ? 'Edit' : 'View', | |
| mode: modeLabel, | |
| const title = t("{{mode}} Minister's Housing Allowance Request", { | |
| mode: type, |
| : mode[1] === PageEnum.Edit | ||
| ? PageEnum.Edit | ||
| : PageEnum.View; | ||
| const type = getPageType(mode); |
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.
optional:
I might make this variable name a little more descriptive
| const type = getPageType(mode); | |
| const pageType = getPageType(mode); |
|
|
||
| const { data, error: requestsError } = | ||
| useMinistryHousingAllowanceRequestsQuery(); | ||
| const requestsData = data?.ministryHousingAllowanceRequests.nodes || []; |
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.
You could update this and remove line 56 const requests = requestsData ?? [];
| const requestsData = data?.ministryHousingAllowanceRequests.nodes || []; | |
| const requests = data?.ministryHousingAllowanceRequests.nodes ?? []; |
| return mode | ||
| ? `/accountLists/${accountListId}/reports/housingAllowance/${requestId}?mode=${mode}` | ||
| : `/accountLists/${accountListId}/reports/housingAllowance/${requestId}`; |
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.
We could probably dry this up a bit
| return mode | |
| ? `/accountLists/${accountListId}/reports/housingAllowance/${requestId}?mode=${mode}` | |
| : `/accountLists/${accountListId}/reports/housingAllowance/${requestId}`; | |
| const baseUrl = `/accountLists/${accountListId}/reports/housingAllowance/${requestId}`; | |
| return mode ? `${baseUrl}?mode=${mode}` : baseUrl; |
| ); | ||
| const mhaRequestId = newRequest?.ministryHousingAllowanceRequest.id; | ||
| const requestLink = `/accountLists/${accountListId}/reports/housingAllowance/${mhaRequestId}/new`; | ||
| const requestLink = getRequestUrl(accountListId, mhaRequestId, 'new'); |
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.
We could probably use the PageEnum here
| const requestLink = getRequestUrl(accountListId, mhaRequestId, 'new'); | |
| const requestLink = getRequestUrl(accountListId, mhaRequestId, PageEnum.New); |
| <Receipt | ||
| formTitle={t('MHA Request')} | ||
| buttonText={t('View Your MHA')} | ||
| editLink={`${getRequestUrl(accountListId, requestId, 'edit')}`} |
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'd pass the enums here for editLink and viewLink.
| useEffect(() => { | ||
| if (type === PageEnum.Edit) { | ||
| setCurrentStep(StepsEnum.RentOrOwn); | ||
| } | ||
| }, [type]); |
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.
Are we sure we want to start edit mode on the second step?
| iconColor="success.main" | ||
| linkOneText={t('View Current MHA')} | ||
| linkOne={`/accountLists/${accountListId}/reports/housingAllowance/${requestId}/view`} | ||
| linkOne={getRequestUrl(accountListId, requestId, 'view')} |
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.
Maybe enum instead of 'view' here.
| iconColor="warning.main" | ||
| linkOneText={t('View Request')} | ||
| linkOne={`/accountLists/${accountListId}/reports/housingAllowance/${requestId}/view`} | ||
| linkOne={getRequestUrl(accountListId, requestId, 'view')} |
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.
Same here on and on linkTwo.
| complete: false, | ||
| }, | ||
| ] | ||
| ? isEdit |
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 wonder if there's a way we could DRY this up a bit. Seems like a lot of repeated code.
Description
Combine new, edit, and view pages into a single page that routes to
/housingAllowance/[requestId]. This page displays different UI based on the mode query parameter.Implementation: I have used query params (?mode=new, ?mode=edit, ?mode=view) to determine which UI to display. I was thinking about using
status(MhaStatusEnum), but there wouldn't be a solid way to determine which display is needed. When a user clicks the navigation buttons, they will be directed to the appropriate mode.NOTE: In order to test this properly, you need to change
MinisterHousingAllowance.tsxline 96 to this:It has been kept as
requests[0]since changing it will affect the tests.Testing
/reports/housingAllowanceChecklist:
/pr-reviewcommand locally and fixed any relevant suggestions