-
Notifications
You must be signed in to change notification settings - Fork 97
feat(modals): support keeping TooltipDialog mounted when closed
#2050
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
Conversation
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.
Nice, that's a clean way to do it
| offset: _offset, | ||
| onClose, | ||
| hasArrow = true, | ||
| keepMounted = false, |
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.
| keepMounted = false, | |
| keepMounted, |
Please follow the Garden convention of defaulting boolean props undefined
| style={{ | ||
| ...backdropProps?.style, | ||
| ...(isHidden | ||
| ? { visibility: 'hidden', pointerEvents: 'none' } | ||
| : { visibility: undefined }) | ||
| }} |
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.
Can these be applied to the StyledTooltipDialogBackdrop CSS rather than inlining the styles?
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.
Also, I'm a bit concerned about leaving the backdrop in place as it there might be some unanticipated stacking issues. If the content is meant to remain accessibly aria-hidden to screen readers, I'd recommend polished JS hideVisually(). Otherwise, it might be good to apply display: contents;.
| focusOnMount: true, | ||
| hasArrow: true, | ||
| restoreFocus: true, | ||
| keepMounted: false, |
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.
| keepMounted: false, |
Prefer Storybook's implicit prop definition whenever possible
| argTypes={{ | ||
| referenceElement: { control: false }, | ||
| fallbackPlacements: { control: 'multi-select', options: PLACEMENT.filter(p => p !== 'auto') }, | ||
| keepMounted: { control: 'boolean' }, |
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.
| keepMounted: { control: 'boolean' }, |
ditto .. should not be needed
| opacity: 0; | ||
| } | ||
| ${props => props.$isHidden && hideVisually()} |
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.
| ${props => props.$isHidden && hideVisually()} | |
| ${props => props['aria-hidden'] && hideVisually()} |
Can we use the ARIA attribute rather than adding a custom prop? This follows the more prominent styling from Garden's more mature components (dropdowns, for example)
| {...(getBackdropProps() as HTMLAttributes<HTMLDivElement>)} | ||
| {...backdropProps} | ||
| ref={transitionRef} | ||
| style={backdropProps?.style} |
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.
| style={backdropProps?.style} |
This shouldn't be needed as it is already spread above.
Description
This PR adds a
keepMountedprop to theTooltipDialogcomponent, allowing the dialog content to remain in the DOM when closed instead of being unmounted. This is useful for preserving component state and improving re-opening performance.Details
keepMountedprop lets you control mounting behaviortrue, the dialog stays in the DOM but gets hidden with CSS and proper ARIA attributesChecklist
👌 design updates will be Garden Designer approved (add the designer as a reviewer)npm start)⚫ renders as expected in dark mode🤘 renders as expected with Bedrock CSS (?bedrock)