-
Notifications
You must be signed in to change notification settings - Fork 44
Feat/add modal window #106
base: main
Are you sure you want to change the base?
Conversation
"@testing-library/jest-dom": "^5.16.5", | ||
"@testing-library/react": "^14.0.0", | ||
"@testing-library/react-hooks": "^8.0.1", | ||
"@testing-library/user-event": "^14.4.3", |
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.
Deps for unit tests.
"prettier": "2.7.1", | ||
"tailwindcss": "^3.3.2", |
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.
Decided to use tailwindcss as i assumed the focus was more on the modal js code more than how it looks.
onClose, | ||
children, | ||
}) => { | ||
// eslint-disable-next-line react/hook-use-state |
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.
Given that i was on a rush, and was not sure how much i should respect the original configs lint rules i had to disable some rules here and there, i know it should be clean.... but i was trying to get the code out. :)
popModal(); | ||
} | ||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
}, [isOpen, id]); |
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.
here there are some pending deps, but the react render cycle was getting out of hand, so i had to limit it to what was relevant, again i think some strict rules should be ease in this kinda of case, given that we should add direct deps
onClick={(event) => { | ||
if ( | ||
event.target === modalRef.current && | ||
modalStack[modalStack.length - 1] === id | ||
) { | ||
onClose(); | ||
} | ||
}} |
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.
Seen this, i would def. move this out... trying to keep it clean and readable.
aria-modal="true" | ||
className="fixed inset-0 bg-neutral-800 bg-opacity-50 " | ||
ref={modalRef} | ||
role="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.
use this role here for more semantic represenation and accesibility, the inner div with presentation role was added given the linting rules.
@@ -0,0 +1,42 @@ | |||
import React, { |
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.
This file tracks the stacking, and is share, i would also abstract the stacking mechanism out of the context and then just use it here to track its state, but the logic for poping, pushing, or swaping could be handle on its own hook.
@@ -0,0 +1,49 @@ | |||
import { renderHook, act } from '@testing-library/react-hooks'; |
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.
unit testing the component, there was a warning/error about render been deprecated, i didnt want to spend too much time on that, sorry about that .
@@ -0,0 +1,38 @@ | |||
import { useEffect, useRef } from 'react'; | |||
|
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.
Core file, this is the modal mechanism. It tracks and handle events such as escaping. And then then also scroll-locking ;).
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.
and i forgot focus.
|
||
const Index: NextPage = () => { | ||
return <h1>Welcome to Contra!</h1>; | ||
const [modalAOpen, setModalAOpen] = useState(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.
on this file i was trying to make it so we were able to open a modal form within a modal, (which is not such a good UX practice), but the goal was to test couple of edge cases. I also had buttons side by side opening or closing modals.
I tried to make the commits as clear as possible.
https://www.loom.com/share/abefecd209c64961b88d563bcaf72540