Skip to content
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

MPDX-8351 Add UseLocalizedConstants() hook #1145

Merged
merged 14 commits into from
Oct 21, 2024

Conversation

caleballdrin
Copy link
Contributor

@caleballdrin caleballdrin commented Oct 18, 2024

Description

https://jira.cru.org/secure/RapidBoard.jspa?rapidView=3&view=detail&selectedIssue=MPDX-8351&sprint=1276#

  • Add new UseLocalizedConstants hook and replace getLocalizedStatus
  • Add API constants mock to the test setup and remove old LoadConstants mocks
  • Move get getContactStatusesByPhase to the useContactPartnershipStatus hook
  • Consolidate functions converting old contact statuses to a new helper file.

Checklist:

  • I have given my PR a title with the format "MPDX-(JIRA#) (summary sentence max 80 chars)"
  • I have applied the appropriate labels. (Add the label "On Staging" to get the branch automatically merged into staging.)
  • I have requested a review from another person on the project

@caleballdrin caleballdrin self-assigned this Oct 18, 2024
@caleballdrin caleballdrin added the Preview Environment Add this label to create an Amplify Preview label Oct 18, 2024
@caleballdrin caleballdrin requested a review from canac October 18, 2024 17:56
Copy link
Contributor

Copy link
Contributor

github-actions bot commented Oct 18, 2024

Bundle sizes [mpdx-react]

Compared against 86d678b

Dynamic import Size (gzipped) Diff
../src/components/Task/MassActions/EditTasks/DynamicMassActionsEditTasksModal.tsx -> ./MassActionsEditTasksModal 99.55 KB +1.5 KB
../src/components/Task/Modal/Form/Complete/DynamicTaskModalCompleteForm.tsx -> ./TaskModalCompleteForm 112.73 KB +1.55 KB
../src/components/Task/Modal/Form/DynamicTaskModalForm.tsx -> ./TaskModalForm 120.84 KB +1.55 KB
../src/components/Task/Modal/Form/LogForm/DynamicTaskModalLogForm.tsx -> ./TaskModalLogForm 151.46 KB +1.55 KB

Copy link
Contributor

@canac canac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work! I love how many lines were deleted, how our tests can be a little simpler now, and that you were able to use basically the same function and just return it from the hook now.

src/components/Constants/LoadConstantsMock.ts Show resolved Hide resolved
src/components/Constants/UseApiConstants.test.tsx Outdated Show resolved Hide resolved
src/utils/functions/convertContactStatus.ts Outdated Show resolved Hide resolved
src/utils/functions/convertContactStatus.ts Outdated Show resolved Hide resolved
src/components/Shared/Filters/FilterPanel.tsx Show resolved Hide resolved
src/hooks/useContactPartnershipStatuses.ts Outdated Show resolved Hide resolved
src/hooks/useLocalizedConstants.ts Outdated Show resolved Hide resolved
@caleballdrin caleballdrin requested a review from canac October 19, 2024 03:21
Copy link
Contributor

@canac canac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good! I think once we refresh the constants when the language changes, this will be good to go.

@caleballdrin caleballdrin requested a review from canac October 21, 2024 17:53
Copy link
Contributor

@canac canac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks great and is exactly what I had in mind! I'd like to spend some time testing it in preview, then I'll approve it.

@caleballdrin caleballdrin requested a review from canac October 21, 2024 19:43
@canac
Copy link
Contributor

canac commented Oct 21, 2024

In preview, it's loading the constants query every time you load the page now. I think it's because useLocalStorage returns initialValue on the first render. I would either update useLocalStorage useState to initialize to readValue() or just directly read from local storage without using the hook.

@canac
Copy link
Contributor

canac commented Oct 21, 2024

I tested all of the components with non-trivial changes in preview, and everything is looking great with the translated statuses.

@caleballdrin caleballdrin requested a review from canac October 21, 2024 20:43
Copy link
Contributor

@canac canac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the PRs I just merged created a merge conflict, sorry! But this looks great now! I'm seeing the constants query whenever you change the language but otherwise using the cached constants.

@caleballdrin caleballdrin merged commit 1180b32 into main Oct 21, 2024
17 of 18 checks passed
@caleballdrin caleballdrin deleted the MPDX-8351-constants-localized branch October 21, 2024 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Preview Environment Add this label to create an Amplify Preview
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants