-
Notifications
You must be signed in to change notification settings - Fork 7
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
Re-implement Modal
component using HTMLDialogElement (#461)
#544
base: master
Are you sure you want to change the base?
Changes from all commits
77224ee
8d6ac22
64ae3d4
06a7018
6348886
4f137dd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I checked the "Launch modal as form" example and found out there is no There is the https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog#usage_notes |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1065,27 +1065,29 @@ accessibility. | |
|
||
## Theming | ||
|
||
| Custom Property | Description | | ||
|------------------------------------------------------|---------------------------------------------------------------| | ||
| `--rui-Modal__padding-x` | Inline padding of individual modal components | | ||
| `--rui-Modal__padding-y` | Block padding of individual modal components | | ||
| `--rui-Modal__background` | Modal background (including `url()` or gradient) | | ||
| `--rui-Modal__box-shadow` | Modal box shadow | | ||
| `--rui-Modal__separator__width` | Width of separator between modal header, body, and footer | | ||
| `--rui-Modal__separator__color` | Color of separator between modal header, body, and footer | | ||
| `--rui-Modal__outer-spacing-xs` | Spacing around modal, `xs` screen size | | ||
| `--rui-Modal__outer-spacing-sm` | Spacing around modal, `sm` screen size and bigger | | ||
| `--rui-Modal__header__gap` | Modal header gap between children | | ||
| `--rui-Modal__footer__background` | Modal footer background (including `url()` or gradient) | | ||
| `--rui-Modal__footer__gap` | Modal footer gap between children | | ||
| `--rui-Modal__backdrop__background` | Modal backdrop background (including `url()` or gradient) | | ||
| `--rui-Modal--auto__min-width` | Min width of auto-sized modal (when enough screen estate) | | ||
| `--rui-Modal--auto__max-width` | Max width of auto-sized modal (when enough screen estate) | | ||
| `--rui-Modal--small__width` | Width of small modal | | ||
| `--rui-Modal--medium__width` | Width of medium modal | | ||
| `--rui-Modal--large__width` | Width of large modal | | ||
| `--rui-Modal--fullscreen__width` | Width of fullscreen modal | | ||
| `--rui-Modal--fullscreen__height` | Height of fullscreen modal | | ||
| Custom Property | Description | | ||
|------------------------------------------------------|-------------------------------------------------------------| | ||
| `--rui-Modal__padding-x` | Inline padding of individual modal components | | ||
| `--rui-Modal__padding-y` | Block padding of individual modal components | | ||
| `--rui-Modal__background` | Modal background (including `url()` or gradient) | | ||
| `--rui-Modal__box-shadow` | Modal box shadow | | ||
| `--rui-Modal__separator__width` | Width of separator between modal header, body, and footer | | ||
| `--rui-Modal__separator__color` | Color of separator between modal header, body, and footer | | ||
| `--rui-Modal__outer-spacing-xs` | Spacing around modal, `xs` screen size | | ||
| `--rui-Modal__outer-spacing-sm` | Spacing around modal, `sm` screen size and bigger | | ||
| `--rui-Modal__header__gap` | Modal header gap between children | | ||
| `--rui-Modal__footer__background` | Modal footer background (including `url()` or gradient) | | ||
| `--rui-Modal__footer__gap` | Modal footer gap between children | | ||
| `--rui-Modal__backdrop__background` | Modal backdrop background (including `url()` or gradient) | | ||
| `--rui-Modal--auto__min-width` | Min width of auto-sized modal (when enough screen estate) | | ||
| `--rui-Modal--auto__max-width` | Max width of auto-sized modal (when enough screen estate) | | ||
| `--rui-Modal--small__width` | Width of small modal | | ||
| `--rui-Modal--medium__width` | Width of medium modal | | ||
| `--rui-Modal--large__width` | Width of large modal | | ||
| `--rui-Modal--fullscreen__width` | Width of fullscreen modal | | ||
| `--rui-Modal--fullscreen__height` | Height of fullscreen modal | | ||
| `--rui-Modal__animation__duration` | Duration of animation used (when opening modal) | | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Currently, closing of @mbohal Do we also want animated closing? That would mean delayed removal from the DOM. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would like to add one thing. Closing animation is hard to impelement. We do not have close handler, we have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After a fruitless afternoon I concur with @bedrich-schindler that:
However, this change is not BC in terms of usage, so I think we should merge it as is. We can add hide animation int he future. This will very likely change the API though… There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On video call agreed that for the time being we will only support opening animation. |
||
|
||
|
||
[button-attributes]: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#attributes | ||
[div-attributes]: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/div#attributes | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
@keyframes fade-in { | ||
0% { | ||
opacity: 0; | ||
} | ||
|
||
100% { | ||
opacity: 1; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
export const dialogOnCancelHandler = (e, closeButtonRef) => { | ||
// Prevent the default behaviour of the event as we want to close dialog manually. | ||
e.preventDefault(); | ||
|
||
// If the close button is not disabled, close the modal. | ||
if ( | ||
closeButtonRef?.current != null | ||
&& closeButtonRef?.current?.disabled === false | ||
) { | ||
closeButtonRef.current.click(); | ||
} | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
export const dialogOnClickHandler = ( | ||
e, | ||
closeButtonRef, | ||
dialogRef, | ||
allowCloseOnBackdropClick, | ||
) => { | ||
// If it is not allowed to close modal on backdrop click, do nothing. | ||
if (!allowCloseOnBackdropClick) { | ||
return; | ||
} | ||
|
||
// Detection of the click on the backdrop is based on the following conditions: | ||
// 1. The click target is the dialog itself. This prevents detection of clicks on the dialog's children. | ||
// 2. The click is outside the dialog's boundaries. | ||
const dialogRect = dialogRef.current.getBoundingClientRect(); | ||
const isClickedOnBackdrop = dialogRef.current === e.target && ( | ||
e.clientX < dialogRect.left | ||
|| e.clientX > dialogRect.right | ||
|| e.clientY < dialogRect.top | ||
|| e.clientY > dialogRect.bottom | ||
); | ||
|
||
// If user does not click on the backdrop, do nothing. | ||
if (!isClickedOnBackdrop) { | ||
return; | ||
} | ||
|
||
// If the close button is not disabled, close the modal. | ||
if (closeButtonRef?.current != null && closeButtonRef?.current?.disabled === false) { | ||
closeButtonRef.current.click(); | ||
} | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
export const dialogOnCloseHandler = (e, closeButtonRef) => { | ||
// Prevent the default behaviour of the event as we want to close dialog manually. | ||
e.preventDefault(); | ||
|
||
// If the close button is not disabled, close the modal. | ||
if (closeButtonRef?.current != null && closeButtonRef?.current?.disabled === false) { | ||
closeButtonRef.current.click(); | ||
} | ||
}; |
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.
I'd expect more deletions than additions. 😞 Can we get rid of some more
featurescustom code in our focus management? Or anywhere else?