-
Notifications
You must be signed in to change notification settings - Fork 14
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
[WIP] feat: publishing of completed documents #11
Conversation
4baf4ad
to
6c15aa0
Compare
3e0edea
to
0692f02
Compare
cf08adf
to
d01fccb
Compare
|
||
return ( | ||
<> | ||
{show && ( |
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.
Don't have to pass a show props in if it's handled by the parent
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.
Can remove the fragment while at it
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.
ok
return ( | ||
<> | ||
{show && ( | ||
<ModalDialog show={show} close={closeDeleteModal}> |
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.
Can remove show props. See comments below on the ModalDialog.
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.
ok
it("should not display anything on initial", () => { | ||
render(<DeleteModal deleteForm={() => {}} show={false} closeDeleteModal={() => {}} />); | ||
|
||
expect(screen.queryAllByText("Delete Form")).toHaveLength(0); | ||
}); |
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.
Can remove when we remove show
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.
still needs it for the part to show the component or not.
}) => { | ||
return ( | ||
<> | ||
{show && ( |
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 thing, can remove show here as well. Managed by whoever calls the dialog
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.
erm because we using the useLockBodyScroll, if the component is being rendered but not shown on screen, the body will be locked, so still need the show props in here, but can remove in the modalDialog.
onFormSubmit, | ||
}) => { | ||
return ( | ||
<> |
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.
Can rm
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 also like to put fragments everywhere lol
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.
hahah ok
return (hasError = true); | ||
} | ||
setFormError(ajv.errors); | ||
return (hasError = false); |
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.
Consider a loop that runs twice.
The first run, return errors
The second run, returns no errors
Should the final hasError be true or false?
In this case it seeemed like if the last loop has no error, then it will return no error.
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.
hmmm ok will edit it to cater to the error in any run of the loop
const validateCurrentForm = (): boolean => { | ||
const ajv = new Ajv(); | ||
let hasError = false; | ||
forms.forEach((form) => { |
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.
Why not just run for the current form?
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.
hmm ok will change to just the current form
const onBackToFormSelection = (): void => { | ||
removeCurrentForm(); | ||
}; |
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.
can remove, it does nothing more than just removeCurrentForm
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.
ok
<SvgIconXCircle /> | ||
</SvgIcon> | ||
<div className="text-red text-xl text-center"> | ||
This form has errors. Please fix the errors and submit again. |
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.
Don't you want to spell the errors out?
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.
yup, will display a list of errors upon validation
<> | ||
{show && ( |
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.
Can rm show
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.
ok
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.
LGTM
Publishing of documents on the blockchain
Done
Not Done
Changes to core
jest-environment-jsdom-sixteen
to test environment to preventTypeError: MutationObserver is not a constructor
error (waitFor error "MutationObserver is not a constructor" with latest version testing-library/dom-testing-library#477)