-
Notifications
You must be signed in to change notification settings - Fork 527
fronted(modal): fix the position of modal overlay #2473
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
Changes from all commits
01d6d15
2d3c081
89cdd78
d1006ec
801b196
4ef6c3a
d719f60
6fe2654
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,33 +1,55 @@ | ||
| import React from 'react' | ||
| import { Modal } from 'react-overlays' | ||
| import React, { useEffect } from 'react' | ||
| import { createPortal } from 'react-dom' | ||
|
|
||
| type ModalProps = React.ComponentProps<typeof Modal> | ||
|
|
||
| export interface OverlayProps extends Omit<ModalProps, 'renderBackdrop' | 'onHide'> { | ||
| export interface OverlayProps { | ||
| show: boolean | ||
| onLeave: () => void | ||
| hidden: boolean | ||
| hidden?: boolean | ||
| className?: string | ||
| children?: React.ReactNode | ||
| } | ||
|
|
||
| const Overlay: React.FC<OverlayProps> = ({ children, show, onLeave, className = '', hidden, ...props }) => { | ||
| const renderBackdrop: React.FC<React.HTMLAttributes<HTMLDivElement>> = (props) => ( | ||
| <div className='fixed top-0 left-0 right-0 bottom-0 bg-black o-50' hidden={hidden} {...props} /> | ||
| ) | ||
| const Overlay: React.FC<OverlayProps> = ({ children, show, onLeave, className = '', hidden }) => { | ||
| useEffect(() => { | ||
| if (!show) return | ||
| const handleEscape = (e: KeyboardEvent) => { | ||
| if (e.key === 'Escape') { | ||
| onLeave() | ||
| } | ||
| } | ||
| document.addEventListener('keydown', handleEscape) | ||
| return () => document.removeEventListener('keydown', handleEscape) | ||
| }, [show, onLeave]) | ||
|
|
||
| if (!show) return null | ||
|
|
||
| return ( | ||
| // Note: react-overlays Modal manages its own portal and positioning. | ||
| // The Modal child component uses fixed positioning to center itself. | ||
| // onHide handles both backdrop clicks and escape key presses. | ||
| <Modal | ||
| {...props} | ||
| show={show} | ||
| backdrop={true} | ||
| className={`${className} z-max`} | ||
| renderBackdrop={renderBackdrop} | ||
| onHide={onLeave}> | ||
| {children} | ||
| </Modal> | ||
| const overlay = ( | ||
| <> | ||
| <div | ||
| className='fixed top-0 left-0 right-0 bottom-0 bg-black o-50' | ||
| hidden={hidden} | ||
| onClick={onLeave} | ||
| onKeyDown={(e) => e.key === 'Enter' && onLeave()} | ||
| role="button" | ||
|
Member
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. nit: The backdrop has A full-screen invisible button is confusing for screen readers. A backdrop click-to-dismiss is standard UX but shouldn't be announced as a button. In the future use |
||
| tabIndex={0} | ||
| aria-label="Close modal" | ||
| style={{ zIndex: 9998 }} | ||
| /> | ||
|
|
||
| <div | ||
| className={`${className} fixed top-0 left-0 right-0 bottom-0 flex justify-center items-center`} | ||
| style={{ zIndex: 9999, pointerEvents: 'none', padding: '2rem' }} | ||
| role="dialog" | ||
| aria-modal="true" | ||
| > | ||
| <div style={{ pointerEvents: 'auto', maxWidth: '100%', maxHeight: '100%', overflow: 'auto' }}> | ||
| {children} | ||
| </div> | ||
| </div> | ||
| </> | ||
| ) | ||
|
|
||
| return createPortal(overlay, document.body) | ||
| } | ||
|
|
||
| export default Overlay | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -110,6 +110,12 @@ test.describe('IPNS publishing', () => { | |
| await files.dialogInput(page, 'name').fill(testFilename) | ||
| await page.keyboard.press('Enter') | ||
|
|
||
| // Wait for the dialog to close before proceeding | ||
| await expect(pathInput).not.toBeVisible({ timeout: 10000 }) | ||
|
|
||
| // Also wait for the modal overlay to be completely gone | ||
| await page.locator('[aria-label="Close modal"]').waitFor({ state: 'hidden', timeout: 5000 }).catch(() => {}) | ||
|
Member
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. nit: Silently swallowing test failures masks real bugs. If the modal overlay doesn't close, the test should fail, not silently continue. |
||
|
|
||
| // expect file with matching filename to be added to the file list | ||
| const fileRow = page.getByTestId('file-row').filter({ hasText: testFilename }) | ||
| await expect(fileRow).toBeVisible() | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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'm confused... when
hidden={true}, the backdrop div gets hidden attribute (invisible) but the content container and its children remain fully visible and interactive.This means the backdrop is invisible but still in the DOM receiving clicks. This seems like unintended behavior? if the overlay is hidden, the whole thing should probably not render at all, no?
(Or maybe I did not look close enough.. anyway.. this feels too complex)