Image preview modal for mobile breakpoint #2470
Conversation
| overlay={true} | ||
| zIndex={theme.zIndex.highlightSummaryPopup} | ||
| /> | ||
| <ModalWrapper aria-modal='true'> |
There was a problem hiding this comment.
should this also have role=dialog?
|
|
||
| export function MediaModalPortal(): React.ReactPortal | null { | ||
| const [isOpen, setIsOpen] = React.useState(false); | ||
| const [modalContent, setModalContent] = React.useState<ReactNode>(null); |
There was a problem hiding this comment.
needs to handle esc to close
const setClosed = React.useCallback(() => setIsOpen(false), [setIsOpen]);
useOnEsc(isOpen, setClosed);
| `; | ||
|
|
||
| // tslint:disable-next-line:variable-name | ||
| const ModalWrapper = styled.div` |
There was a problem hiding this comment.
this sorta needs to be taking up the whole screen so that it can center things properly, but its gobbling click events so clicking on the overlay isn't closing things. i think you can set a css prop on here to allow click events to pass through and that'll fix it
There was a problem hiding this comment.
this whole MediaModalManger setup is pretty convoluted, can you look at the highlightManager and try to share some of the patterns?
don't need the singleton class. stylistically, would be more consistent if it was a function and not a class. PageComponent can construct the manager and store the manager reference
PageComponent should orchestrate managers but there shouldn't be any business logic in there, move all the event listeners into the manager, you can bind your listener to the page component element instead of each individual image so that you don't have to re-apply the listeners when the content changes
| return <MinPageHeight> | ||
| <this.highlightManager.CardList /> | ||
| <PT /> | ||
| <MediaModalPortal /> |
There was a problem hiding this comment.
if you look above, the CardList is coming from the manager so that you don't need all the wonky binding logic. if you prefer, you could leave the component separate and pass the manager into it as a prop here, but either of those approaches would be more clear than the singleton binding its doing now
| useEffect(() => { | ||
| setModalContent = (content) => { | ||
| setContent(content); | ||
| setIsOpen(true); | ||
| }; | ||
| return () => { setModalContent = null; }; | ||
| }, []); | ||
| if (typeof document === 'undefined') return null; | ||
|
|
||
| return isOpen ? createPortal( | ||
| <MediaModal isOpen={isOpen} onClose={() => setIsOpen(false)}> | ||
| {modalContent} | ||
| </MediaModal>, | ||
| document.body | ||
| ) : null; |
There was a problem hiding this comment.
did you look at this? comment still seems outstanding
3 participants
https://openstax.atlassian.net/browse/CORE-652