-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat(ui): userPrompt
#713
Conversation
return userPromptState.create(newuserPrompt); | ||
}; | ||
|
||
export const UserPrompt = () => { |
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.
Function UserPrompt
has a Cognitive Complexity of 8 (exceeds 5 allowed). Consider refactoring.
Code Climate has analyzed commit 9ba4c3d and detected 1 issue on this pull request. Here's the issue category breakdown:
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. |
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.
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?
@13bfrancis since they'll be both tied to the same So two dialogs, but the same content. |
🎉 This PR is included in version 1.5.0-val.71 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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: aProvider
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 auserPrompt
from anywhere in the code, regardless of where it's defined. This also means we can testuserPrompt
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.data-testid
attributes toDialog
for testingConfirmationModal
toConfirmationDialog
to better reflect its childrenConfirmationDialog
into its own folderuserPrompt