This repository has been archived by the owner on Dec 20, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This is an app that only contains a custom Modal with web accessibility.
Main features: (Demo in the video too, if you want to skip this part)
Tab
to move to next element (Cancel & cross button)Demo
https://www.loom.com/share/c6ea9737128a42f8b155e6bac67b304b
Implementation process
The following steps were taken to work on the app:
How to setup
git clone [email protected]:iamseye/contra-assessment.git
cd contra-assessment
yarn dev
Running tests
yarn test
Decisions and tradeoffs
Dynamic React portal
React portal is designed to create another DOM on the same layer of the app DOM. But instead of having another
<div id="modal"/>
in HTML. I created a ReactPortal component to dynamic and added the<div />
when the modal opened. Remove it when the modal is close to avoid redundancy.Opening multiple modals
Open multiple modals are not recommended based on the UX perspective. It can lead to confusion for the user, especially if there are multiple layers of modals. It may also cause clutter on the screen, making it difficult for the user to focus on the primary task. If needed in the real situation, I would prefer to close the first modal before opening the second modal instead of having multiple layers.
But in this case, there's a stacking modal requirement, so the second modal is opening on top of the first modal.
Modal props
It may seem props
isOpen
andhandleClose
functions can be not necessary. butisOpen
before rendering the modal can avoid rendering non-used hooks in the component.ex:
handleClose
can props custom actions but not only close the modal.Modal management
I was considering having a ModalContext to centralize the modal management. If it's necessary to have more than 3 layers of modals, it can lead to having too much logic just to control the isOpen & trigger elements. But using a centralized context approach also means every time added or remove modal needs to rerender all the models, and also needs to keep tracking the orders. I didn't do it this time, considering if we do have context management, should also have maximize 3 layers of the modals to warn the devs from the UX perspective.
If it was a bigger project that I think I can improve further
Something else
I'm having the same issue while using
esling-config-canonical
, I haven't got time to look into it. but some of my linters may not be 100% followed.