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

feat(ui): userPrompt #713

Merged
merged 5 commits into from
Aug 7, 2024
Merged

feat(ui): userPrompt #713

merged 5 commits into from
Aug 7, 2024

Conversation

asharonbaltazar
Copy link
Collaborator

Purpose

To prepare the codebase for the eventual forms revamp and lay the ground for best test practices, I'm making the transition easier by removing our reliance on React-centric logic. ModalContext relied on two things: a Provider and React's rules of hooks, both limiting what we can do when it comes to defining user prompts.

Linked Issues to Close

https://qmacbis.atlassian.net/browse/OY2-29584

Approach

We needed to ditch Provider. After some research and experimentation, I think using an observer pattern is the best bet. Sure, it requires some setup, but the advantage is now we can create a userPrompt from anywhere in the code, regardless of where it's defined. This also means we can test userPrompt and spy on whether it's called, just like any function, and thus reducing the complexity for our tests without the setup that context and hooks require.

  • add some data-testid attributes to Dialog for testing
  • rename ConfirmationModal to ConfirmationDialog to better reflect its children
  • move ConfirmationDialog into its own folder
  • replace modal context logic with userPrompt

return userPromptState.create(newuserPrompt);
};

export const UserPrompt = () => {
Copy link

Choose a reason for hiding this comment

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

Function UserPrompt has a Cognitive Complexity of 8 (exceeds 5 allowed). Consider refactoring.

Copy link
Contributor

github-actions bot commented Aug 7, 2024

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 53.78% 4251 / 7904
🔵 Statements 53.62% 4474 / 8343
🔵 Functions 48.22% 1016 / 2107
🔵 Branches 27.09% 803 / 2964
File Coverage
File Stmts % Branch % Funcs % Lines Uncovered Lines
Changed Files
react-app/src/components/ConfirmationDialog/userPrompt.test.tsx 100% 100% 100% 100%
react-app/src/components/ConfirmationDialog/userPrompt.tsx 97.72% 80% 92.85% 100% 66
react-app/src/components/Layout/index.tsx 11.36% 0% 0% 12.19% 19-20, 22-24, 23, 27-50, 50, 52, 56, 58-60, 59, 62-64, 63, 66, 66, 120, 175-178, 180-187, 181, 183-186, 189-192, 190-191, 194, 194, 196-199, 197-203, 201-202, 205-245, 293, 293, 304-311
react-app/src/features/package-actions/ActionForm.tsx 3.44% 0% 0% 3.44% 35-38, 40-44, 46-48, 50-52, 51, 54, 56-88, 57-87, 58-64, 66-68, 70-73, 75, 77, 79-86, 90-97, 91-96, 92-95, 105-116, 118
react-app/src/features/package-actions/lib/contentSwitch.ts 55.55% 0% 0% 62.5% 96-108, 108-110, 110-111
react-app/src/features/package/package-details/appk.tsx 3.57% 0% 0% 3.57% 19-26, 25, 28-59, 29-57, 39-51, 40-50, 54-55, 60-63, 62, 89, 103-104
react-app/src/features/submission/waiver/shared-components/SubmitAndCancelBtnSection.tsx 7.69% 0% 0% 8.33% 22-24, 26-29, 27-28, 33-36, 34-35, 34-35, 56, 64-71
Generated in workflow #59

Copy link

codeclimate bot commented Aug 7, 2024

Code Climate has analyzed commit 9ba4c3d and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

The test coverage on the diff in this pull request is 96.5% (50% is the threshold).

This pull request will bring the total coverage in the repository to 53.7% (0.3% change).

View more on Code Climate.

Copy link
Collaborator

@13bfrancis 13bfrancis left a comment

Choose a reason for hiding this comment

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

Wanted to start off with just a question so I understand. The UserPrompt component. Not that there should be, but if there were two of them placed in the DOM what would happen? Not shooting anything down or anything. Just a morbid curiosity question?

@asharonbaltazar
Copy link
Collaborator Author

@13bfrancis since they'll be both tied to the same UserPromptState, they can only display the current dialog in state.

So two dialogs, but the same content.

@asharonbaltazar asharonbaltazar merged commit 8dd3a73 into main Aug 7, 2024
15 of 16 checks passed
@asharonbaltazar asharonbaltazar deleted the userprompt branch August 7, 2024 18:21
Copy link
Contributor

github-actions bot commented Aug 7, 2024

🎉 This PR is included in version 1.5.0-val.71 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants