chore: use radix to improve modal component #854
Conversation
RyukTheCoder
requested review from
Ikari-Shinji-re and
mikasackermn
as code owners
RyukTheCoder
changed the title
RyukTheCoder
changed the title
RyukTheCoder
changed the title
RyukTheCoder
changed the title
RyukTheCoder
force-pushed
the
chore/rf-1777-use-radix-to-improve-modal-component
branch
from
721a7c1 to
8765327
Compare
|
Thanks. Your changes looks good to me. For handling animation Radix seems using data attributes. Here is an example from Shadcn doing samething on top of Radix. Please take a look at the approach and see is there anyway to remove our js approach to handle animations. |
RyukTheCoder
requested review from
yeager-eren
and removed request for
Ikari-Shinji-re,
mikasackermn and
yeager-eren
Please check my latest changes. I used data attributes to handle animations. |
|
In this commit I tried to prevent an error related to |
RyukTheCoder
force-pushed
the
chore/rf-1777-use-radix-to-improve-modal-component
branch
from
1fa28fb to
4d55b42
Compare
… handle error for custom header
| }); | ||
|
|
||
| export const DialogTitle = styled(Dialog.DialogTitle, { | ||
| margin: 0, |
There was a problem hiding this comment.
I think this will affect a11y and they did the trick for that. We can ignore 1px difference.
There was a problem hiding this comment.
This was not added to handle 1px difference. Actually with position set to absolute, there is no 1px difference. When we wrap title with Dialog.DialogTitle it will be wrapped in a h2 element which have a default margin so I added this to remove the unwanted margin.
yeager-eren
left a comment
yeager-eren
left a comment
There was a problem hiding this comment.
Thanks.
Since we've opened this PR some time ago, please make sure the dependencies are in their latest version.
| // Restrict the overlay click area to just the overlay element, rather than the entire page | ||
| const handleOverlayClick: MouseEventHandler = (event) => { | ||
| if (event.target === overlayRef.current) { | ||
| handleBackDropClick(!open); |
There was a problem hiding this comment.
handleBackDropClick is only used here, so you can remove the function and move its code block to here.
|
|
||
| if (event.target === event.currentTarget && dismissible) { | ||
| // The escape key functionality only works when the focus is within the overlay (modal). | ||
| const handleScapeKeyDown = () => { |
There was a problem hiding this comment.
| const handleScapeKeyDown = () => { | |
| const handleEscapeKeyDown = () => { |
| <Dialog.Portal container={container}> | ||
| <DialogOverlay ref={overlayRef} onClick={handleOverlayClick}> | ||
| <DialogContent | ||
| onClick={handleContentClick} |
There was a problem hiding this comment.
Instead of hacking with onClick and :focus, you can consider using onInteractOutside.
ref
It will be like:
onInteractOutside={handleOverlayClick}
Ensure renaming the function and also remove the onClicks.
|
Currently, we have a Modal component in our design system package. However, our implementation works differently depending on its container. Based on our requirements for our widget and other applications, we have identified two types of modals: A variant that occupies the entire viewport, blocking user interaction outside its content. These libraries position the modal as fixed, blocking the entire viewport, which does not meet our requirement for container-based positioning (e.g., bottom sheets in our widget). To address this, I propose splitting the Modal component into two separate components: Modal: A standard modal built with external packages like Radix UI, incorporating most of the changes made in this pull request. |
|
@Ikari-Shinji-re Thanks. I will close this issue and we will address this in our upcoming design system. |
Summary
Use Radix UI to improve modal component.
In result of this change:
Fixes # 1777
Notes:
Currently, we are handling the logic related to showing animation on opening and closing modal and positioning the content of modal based on
anchorprop. Before exploring the Radix UI document, we were optimistic about that it can handle these animation logics and positioning the content of modal, but after checking the documentation I didn't find such solutions so I kept it as it is.How did you test this change?
Tested this change by checking most of modals.
Checklist: