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/add modal window #106

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions frontend/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,25 @@
"react-dom": "18.2.0"
},
"devDependencies": {
"@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",
Comment on lines +21 to +24
Copy link
Author

Choose a reason for hiding this comment

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

Deps for unit tests.

"@types/jest": "28.1.3",
"@types/node": "18.0.0",
"@types/react": "18.0.14",
"@types/react-dom": "18.0.5",
"autoprefixer": "^10.4.14",
"eslint": "8.18.0",
"eslint-config-canonical": "35.0.1",
"eslint-config-next": "12.1.6",
"eslint-config-prettier": "8.5.0",
"jest": "28.1.1",
"jest-environment-jsdom": "^29.5.0",
"lint-staged": "13.0.3",
"postcss": "^8.4.23",
"prettier": "2.7.1",
"tailwindcss": "^3.3.2",
Copy link
Author

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.

"typescript": "4.7.4"
}
}
6 changes: 6 additions & 0 deletions frontend/postcss.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
module.exports = {
plugins: {
tailwindcss: {},
autoprefixer: {},
},
}
61 changes: 61 additions & 0 deletions frontend/src/components/Modal.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import React, { useEffect } from 'react';
import ReactDOM from 'react-dom';
import { useModalStack } from '@/context/ModalContext';
import { useModal } from '@/hooks/useModal';

type ModalProps = {
children: React.ReactNode;
id: string;
isOpen: boolean;
onClose: () => void;
};

export const Modal: React.FC<ModalProps> = ({
id,
isOpen,
onClose,
children,
}) => {
// eslint-disable-next-line react/hook-use-state
Copy link
Author

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. :)

const modalRef = useModal(isOpen, onClose);
const { modalStack, pushModal, popModal } = useModalStack();

useEffect(() => {
// eslint-disable-next-line no-debugger
if (isOpen) {
pushModal(id);
} else {
popModal();
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [isOpen, id]);
Copy link
Author

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


if (!isOpen) return null;

return ReactDOM.createPortal(
<div
aria-modal="true"
className="fixed inset-0 bg-neutral-800 bg-opacity-50 "
ref={modalRef}
role="dialog"
Copy link
Author

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.

style={{ zIndex: 1_000 + modalStack.indexOf(id) }}
tabIndex={-1}
>
<div
className="w-full h-full flex items-center justify-center"
onClick={(event) => {
if (
event.target === modalRef.current &&
modalStack[modalStack.length - 1] === id
) {
onClose();
}
}}
Comment on lines +46 to +53
Copy link
Author

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.

role="presentation"
>
<div className="min-w-fit bg-white p-4 rounded">{children}</div>
</div>
</div>,
document.body
);
};
42 changes: 42 additions & 0 deletions frontend/src/context/ModalContext.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import React, {
Copy link
Author

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.

type PropsWithChildren,
createContext,
useContext,
useState,
} from 'react';

type ModalContextType = {
modalStack: string[];
popModal: () => void;
pushModal: (modalId: string) => void;
};

const ModalContext = createContext<ModalContextType | null>(null);

export const useModalStack = () => {
const context = useContext(ModalContext);
if (!context) {
throw new Error('useModalStack must be used within a ModalStackProvider');
}

return context;
};

export const ModalStackProvider = ({ children }: PropsWithChildren) => {
const [modalStack, setModalStack] = useState<string[]>([]);

const pushModal = (modalId: string) => {
setModalStack((previousStack) => [...previousStack, modalId]);
};

const popModal = () => {
setModalStack((previousStack) => previousStack.slice(0, -1));
};

return (
// eslint-disable-next-line react/jsx-no-constructed-context-values
<ModalContext.Provider value={{ modalStack, popModal, pushModal }}>
{children}
</ModalContext.Provider>
);
};
49 changes: 49 additions & 0 deletions frontend/src/hooks/__tests__/useModal.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import { renderHook, act } from '@testing-library/react-hooks';
Copy link
Author

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 .

import { useModal } from '../useModal';

describe('useModal', () => {
it('should close modal on Escape key press', () => {
const onClose = jest.fn();
renderHook(() => useModal(true, onClose));

act(() => {
const event = new KeyboardEvent('keydown', { key: 'Escape' });
document.dispatchEvent(event);
});

expect(onClose).toHaveBeenCalledWith();
});

it('should not close modal on other key press', () => {
const onClose = jest.fn();
renderHook(() => useModal(true, onClose));

act(() => {
const event = new KeyboardEvent('keydown', { key: 'Enter' });
document.dispatchEvent(event);
});

expect(onClose).not.toHaveBeenCalled();
});

it('should set overflow style on body when modal is open', () => {
renderHook(() => useModal(true, () => {}));
expect(document.body.style.overflow).toBe('hidden');
});

it('should reset overflow style on body when modal is closed', () => {
const { rerender } = renderHook(
({ isOpen, onClose }) => useModal(isOpen, onClose),
{
initialProps: {
isOpen: true,
onClose: () => {},
},
}
);

rerender({ isOpen: false, onClose: () => {} });

expect(document.body.style.overflow).toBe('unset');
});
});
38 changes: 38 additions & 0 deletions frontend/src/hooks/useModal.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import { useEffect, useRef } from 'react';

Copy link
Author

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 ;).

Copy link
Author

Choose a reason for hiding this comment

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

and i forgot focus.

export const useModal = (
isOpen: boolean,
onClose: () => void
): React.RefObject<HTMLDivElement> => {
const modalRef = useRef<HTMLDivElement>(null);

useEffect(() => {
const closeOnEscapeKey = (event: KeyboardEvent) => {
if (event.key === 'Escape') {
onClose();
}
};

document.addEventListener('keydown', closeOnEscapeKey);

return () => {
document.removeEventListener('keydown', closeOnEscapeKey);
};
}, [onClose]);

useEffect(() => {
if (isOpen) {
// disable scrolling while modal is open
document.body.style.overflow = 'hidden';
// focus on modal element as soon as it is open
modalRef.current?.focus();
}

return (): void => {
// reset overflow of body once modal is closed.
document.body.style.overflow = 'unset';
};
}, [isOpen]);

return modalRef;
};
10 changes: 8 additions & 2 deletions frontend/src/pages/_app.tsx
Original file line number Diff line number Diff line change
@@ -1,8 +1,14 @@
/* eslint-disable canonical/filename-match-exported */
/* eslint-disable canonical/filename-match-exported, import/no-unassigned-import */
import { type AppProps } from 'next/app';
import { ModalStackProvider } from '@/context/ModalContext';
import './globals.css';

const App = ({ Component, pageProps }: AppProps) => {
return <Component {...pageProps} />;
return (
<ModalStackProvider>
<Component {...pageProps} />
</ModalStackProvider>
);
};

export default App;
3 changes: 3 additions & 0 deletions frontend/src/pages/globals.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
@tailwind base;
@tailwind components;
@tailwind utilities;
60 changes: 59 additions & 1 deletion frontend/src/pages/index.tsx
Original file line number Diff line number Diff line change
@@ -1,8 +1,66 @@
/* eslint-disable canonical/filename-match-exported */
import { type NextPage } from 'next';
import { useState } from 'react';
import { Modal } from '@/components/Modal';

const Index: NextPage = () => {
return <h1>Welcome to Contra!</h1>;
const [modalAOpen, setModalAOpen] = useState(false);
Copy link
Author

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.

const [modalBOpen, setModalBOpen] = useState(false);

return (
<div className="p-4">
<h1>Welcome to Contra!</h1>
<h2>Multi-Modal Environment</h2>
<div className="container mx-auto px-4 py-8">
<button
className="bg-blue-500 text-white px-4 py-2 rounded"
onClick={() => setModalAOpen(true)}
type="button"
>
Open Modal A
</button>

<Modal
id="modalA"
isOpen={modalAOpen}
onClose={() => setModalAOpen(false)}
>
<h2 className="text-lg font-bold mb-4">Modal A Content</h2>
<p className="mb-4">This is the content inside Modal A.</p>
<button
className="bg-red-500 text-white px-4 py-2 rounded"
onClick={() => setModalAOpen(false)}
type="button"
>
Close Modal A
</button>
<button
className="bg-blue-500 text-white px-4 py-2 rounded"
onClick={() => setModalBOpen(true)}
type="button"
>
Open Modal B
</button>
</Modal>

<Modal
id="modalB"
isOpen={modalBOpen}
onClose={() => setModalBOpen(false)}
>
<h2 className="text-lg font-bold mb-4">Modal B Content</h2>
<p className="mb-4">This is the content inside Modal B.</p>
<button
className="bg-red-500 text-white px-4 py-2 rounded"
onClick={() => setModalBOpen(false)}
type="button"
>
Close Modal B
</button>
</Modal>
</div>
</div>
);
};

export default Index;
7 changes: 7 additions & 0 deletions frontend/tailwind.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
module.exports = {
content: ['./src/**/*.{js,ts,jsx,tsx,mdx}'],
plugins: [],
theme: {
extend: {},
},
};