LG-4952: Updates Modal component to use dialog element #2579
+560
−477
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: e59a668 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
bruugey
commented
Dec 5, 2024
|
Size Change: -1.56 kB (-0.11%) Total Size: 1.48 MB
ℹ️ View Unchanged
|
bruugey
changed the title
…ui into brooke/modal-updates
stephl3
reviewed
Mar 5, 2025
packages/confirmation-modal/src/ConfirmationModal/ConfirmationModal.spec.tsx
Show resolved
Hide resolved
| }, [portalRef]) | ||
|
|
||
| <Modal portalRef={portalRef}> | ||
| <Select renderMode="portal" portalContainer={containerState.current}/> |
There was a problem hiding this comment.
if we continue using a PortalContextProvider in Modal.tsx, it should improve the consumer experience since they won't need to specify portalContainer in each popover instance that uses renderMode="portal"
There was a problem hiding this comment.
Updated to what I think makes sense but let me know
| }; | ||
|
|
||
| useEffect(() => { | ||
| if (localRef.current != null) { |
There was a problem hiding this comment.
nit: could invert and early return for readability
stephl3
reviewed
Mar 5, 2025
Co-authored-by: Stephen Lee <stephen.lee@mongodb.com>
stephl3
reviewed
Mar 18, 2025
| `; | ||
|
|
||
| export const modalContentStyle = css` | ||
| const dialogContentStyle = (theme: Theme) => css` |
There was a problem hiding this comment.
dialog native styles clip any content that overflows. for the case of popovers that portal or render inline, we may need to override with overflow: visible
There was a problem hiding this comment.
There was a problem hiding this comment.
I believe I fixed it - take a look!
Co-authored-by: Stephen Lee <stephen.lee@mongodb.com>
Co-authored-by: Stephen Lee <stephen.lee@mongodb.com>
stephl3
reviewed
Apr 16, 2025
| opacity: 0; | ||
| transition: opacity ${transitionDuration.default}ms ease-in-out; | ||
| `; | ||
| const animationStyles = (theme: Theme) => css` |
There was a problem hiding this comment.
nit:
Suggested change
| const animationStyles = (theme: Theme) => css` | |
| const getAnimationStyles = (theme: Theme) => css` |
| `; | ||
|
|
||
| export const modalContentStyle = css` | ||
| const dialogContentStyle = (theme: Theme) => css` |
There was a problem hiding this comment.
packages/modal/UPGRADE.md
Outdated
|
|
||
| Modal v18 introduces a new approach to rendering popover elements using the top layer, ensuring they appear above all other page content. This aligns with the top layer solution implemented in Popover v12. | ||
|
|
||
| Previously, modal and popover elements relied on the usePortal boolean prop to determine whether they should render inline or in a portal appended to the body (or a provided portalContainer). This is no longer necessary with the new implementation. |
There was a problem hiding this comment.
renderMode* prop? can we be more explicit in specifying that popover elements relied on it when rendering inline or in a portal and that it's no longer necessary only if consumers have upgraded to the latest versions of LG components?
There was a problem hiding this comment.
We didn't add the renderMode prop to Modal
Comment on lines
-69
to
-74
| /** | ||
| * By default, when a focus trap is activated the first element in the focus trap's tab order will receive focus. | ||
| * With this option you can specify a different element to receive that initial focus. | ||
| * Selector string (which will be passed to document.querySelector() to find the DOM node) | ||
| */ | ||
| initialFocus?: string; |
There was a problem hiding this comment.
I'm not immediately clear on how to best tackle this, but we do need need some prop to handle if consumer wants to autofocus the close button or an element rendered in children
https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/dialog#accessibility
The autofocus attribute needs to be explicitly applied on the close button or on a separate element in the modal. Possible ideas:
- use a boolean prop defaulting to
trueto determine if the close button should be auto-focused. Consumers can explicitly set this tofalseif applyingautofocusto an alternative element - always set
autofocuson close button. rely on browser behavior which will focus the first element ifautofocusis applied to multiple elements. Less explicit and relies onchildrenrendering before<CloseButton>
There was a problem hiding this comment.
Perhaps we discuss this in dev sync today. I was willing to rely on browser behavior (maybe wrongfully believing that this prop was underused by consumers). But if you feel strongly that we should continue to expose an initialFocus option we can talk about what that might/should look like.
There was a problem hiding this comment.
I don't think the browser will automatically focus the close button without specifying autofocus in the CloseButton component. Maybe "need" was the wrong word, and rather, I'm suggesting that we should offer a way to set initial focus on the close button if there isn't an alternative option consumers want to use
|
|
||
| - `contentClassName` – No longer needed, as Modals no longer have two separate wrapper elements. Most use cases can pass the value directly to the className prop. | ||
|
|
||
| - `initialFocus` – The browser now manages focus automatically when the Modal opens. |
There was a problem hiding this comment.
would update this language to match whatever approach we go with for handling autofocus. including an example here might be helpful
Comment on lines
+79
to
+89
| useEffect(() => { | ||
| if (open && localRef.current != null) { | ||
| localRef.current.appendChild(portalContainer.current); | ||
| portalContainer.current.id = 'lg--modal-dialog-portal'; | ||
|
|
||
| // Expose the portal container via the ref | ||
| if (portalRef) { | ||
| portalRef.current = portalContainer.current; | ||
| } | ||
| } | ||
| }, [open, localRef, portalRef]); |
There was a problem hiding this comment.
still feel like there's some redundancy here. I might be missing something and would appreciate checking this out together to better understand
bruugey
commented
Apr 17, 2025
There was a problem hiding this comment.
Do we need to set the 200vh for the scroll story?
There was a problem hiding this comment.
I think it's specified to test scrolling
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
✍️ Proposed changes
🎟 Jira ticket: LG-4952
✅ Checklist
For bug fixes, new features & breaking changes
yarn changesetand documented my changesFor new components
🧪 How to test changes