shadcn-ui / ui

Beautifully designed components that you can copy and paste into your apps. Accessible. Customizable. Open Source.
https://ui.shadcn.com
MIT License
64.51k stars 3.67k forks source link

[Alert] : Headings must have content and the content must be accessible by a screen reader | jsx-a11y/heading-has-content #1983

Open alamenai opened 7 months ago

alamenai commented 7 months ago

Thank you @shadcn for this amazing library. It helped us a lot to improve our UI and reduce the development time of components.

I used the alert component for displaying an important information, however, I found that it does not pass a the accessibility rule by ESLINT A11Y plugin which displays this error:

image

The line of code behind this issue:


<h5
    ref={ref}
    className={cn("mb-1 font-medium leading-none tracking-tight", className)}
    {...props}
  />

Is it safe to disable the rule or should I resolve it within the component?

Thank you

shadcn commented 7 months ago

I think this is a bug that needs to be fix. We should probably change the <h5 /> tag here to something else and not force a heading here. I need to check what's the recommended one for a11y.

dhruva2000 commented 7 months ago

Hey is this issue up for grabs? I'd like to work on it

brettdh commented 7 months ago

I fixed a similar warning on CardTitle (in my generated component code) by changing it to use the children prop explicitly; i.e.

 const CardTitle = React.forwardRef<
   HTMLParagraphElement,
   React.HTMLAttributes<HTMLHeadingElement>
->(({ className, ...props }, ref) => (
+>(({ className, children, ...props }, ref) => (
   <h3
     ref={ref}
     className={cn(
       "text-2xl font-semibold leading-none tracking-tight",
       className,
     )}
     {...props}
-  />
+  >
+    {children}
+  </h3>
 ))
 CardTitle.displayName = "CardTitle"
alamenai commented 7 months ago

@shadcn , what do you think about this suggestion by @brettdh ?

gaurangrshah commented 3 months ago

Same issue with the AlertTitle component

image

gaurangrshah commented 3 months ago

I think this is a bug that needs to be fix. We should probably change the <h5 /> tag here to something else and not force a heading here. I need to check what's the recommended one for a11y.

Might be the naive answer, but I think just replacing the h5 with an adequately styled p tag or a semantically correct strong tag would be the right way to go here, no?

subhoghoshX commented 1 month ago

Just came across this as my ESLint started spitting warnings.

@brettdh's suggestion seems like the way to go. Any strong reason why this isn't the default?

mudiagauwojeya commented 4 days ago

Thanks for these components, helped me out of a pinch. However, now I am getting Headings must have content and the content must be accessible by a screen reader jsx-a11y/heading-has-content because of a Card component. I used @brettdh's solution and it worked.