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.

Sign up for GitHub

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

Jump to bottom

Fix DialogContent requires a DialogTitle warning #62

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

Fix DialogContent requires a DialogTitle warning #62

wants to merge 1 commit into 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 :-)

mainsmirnov
mainsmirnov reviewed Nov 11, 2024
View reviewed changes
src/components/Overlays/Modal/Modal.tsx
@@ -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?

@vuonghuuhung
Copy link

It worked with me, thank for your solution, I need to patch-package it currently.

@backdownof
Copy link

same error

@bragaru-i
Copy link

This issue still persist in @2.1.8

`DialogContent` requires a `DialogTitle` for the component to be accessible for screen reader users.

If you want to hide the `DialogTitle`, you can wrap it with our VisuallyHidden component.

For more information, see https://radix-ui.com/primitives/docs/components/dialog

and

Warning: Missing `Description` or `aria-describedby={undefined}` for {DialogContent}.
overrideMethod @ hook.js:608
n2.<computed> @ eruda.js?v=f112dd21:3361
(anonymous) @ chunk-JLKUWZTE.js?v=f112dd21:2081
commitHookEffectListMount @ chunk-GKJBSOWT.js?v=f112dd21:16915
invokePassiveEffectMountInDEV @ chunk-GKJBSOWT.js?v=f112dd21:18324
invokeEffectsInDev @ chunk-GKJBSOWT.js?v=f112dd21:19701
commitDoubleInvokeEffectsInDEV @ chunk-GKJBSOWT.js?v=f112dd21:19686
flushPassiveEffectsImpl @ chunk-GKJBSOWT.js?v=f112dd21:19503
flushPassiveEffects @ chunk-GKJBSOWT.js?v=f112dd21:19447
commitRootImpl @ chunk-GKJBSOWT.js?v=f112dd21:19416
commitRoot @ chunk-GKJBSOWT.js?v=f112dd21:19277
performSyncWorkOnRoot @ chunk-GKJBSOWT.js?v=f112dd21:18895
flushSyncCallbacks @ chunk-GKJBSOWT.js?v=f112dd21:9119
(anonymous) @ chunk-GKJBSOWT.js?v=f112dd21:18627

perhaps reexporting styled anatomy parts from radix can help user to fix it by they own?
https://www.radix-ui.com/primitives/docs/components/dialog#anatomy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mainsmirnov mainsmirnov mainsmirnov left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Modal error
5 participants
@toplenboren @vuonghuuhung @backdownof @bragaru-i @mainsmirnov