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 role for Dialog component, should be dialog, not alertdialog #11

Merged
merged 1 commit into from
Aug 11, 2022

Conversation

vincentfretin
Copy link
Contributor

I was using the dialog example code, then after reading https://www.w3.org/WAI/ARIA/apg/patterns/dialogmodal/
to better understand the accessibility requirements, I changed to use AlertDialog, then I got the
<DialogOverlay> must be used inside a <Dialog> error not understanding why I got this error. I dug into the code and understood that all the tags needs to be changed to the Alert variant, so AlertDialogOverlay, AlertDialogPanel etc.
Is there a technical difficulty of using the same components and setting the correct role attribute to the DialogControlled/DialogUncontrolled component? Wouldn't it be better to have only one code to maintain?
I compared the code of all those components to find there was actually no difference, excepted some import sorting.
From the documentation linked above, the only difference should be the role, so role="alertdialog" for AlertDialog and role="dialog" for the Dialog component. For Dialog, this is currently wrongly set to role="alertdialog", this PR fixes it.

@vercel
Copy link

vercel bot commented Aug 11, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
solid-headless ❌ Failed (Inspect) Aug 11, 2022 at 1:43PM (UTC)

@lxsmnsyc
Copy link
Owner

Oof, I probably accidentally forgot to rewrite when I copied AlertDialog for when I was rewriting the library. Yes, the only difference is the role. Structure-wise, AlertDialog and Dialog should be the same. Thanks!

@lxsmnsyc lxsmnsyc merged commit 6ca7127 into lxsmnsyc:main Aug 11, 2022
@vincentfretin vincentfretin deleted the fix-dialog-role branch August 17, 2022 11:10
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.

None yet

2 participants