Skip to content
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

Fix DialogContent requires a DialogTitle warning #62

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

toplenboren
Copy link

This should fix #55

image

RadixUI which is used by Vaul, which is used by this library is quite opinionated when it comes to a11y.

It requires to explicitly set <Dialog.Title> (for this library it is <Drawer.TItle>) and <Dialog.Description>

Here is reference issue in Radix: radix-ui/primitives#2986

--

In this PR I just utilize VisuallyHidden and hack my way thorough this warning. The only thing that seems to be added to final DOM is this:

image

--

Thanks for the lib by the way :-) using it to craft a small pet project, works like charm :-)

@@ -108,7 +109,9 @@ export const Modal = forwardRef<HTMLDivElement, ModalProps>(({
ref={ref}
className={classNames(styles.wrapper, className)}
{...restProps}
aria-describedby={undefined}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thank you for your PR.

aria-describedby should be undefined without it. You also rewrite it for restProps in case someone wants to pass it, did you do it on purpose?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modal error
2 participants