Adding empty class attribute to <ul>

[VanTsang888]

Have you read the Contributing Guidelines on issues?

• I have read the Contributing Guidelines on issues.

Prerequisites

• I'm using the latest version of Docusaurus.
• I have tried the npm run clear or yarn clear command.
• I have tried rm -rf node_modules yarn.lock package-lock.json and re-installing packages.
• I have tried creating a repro with https://new.docusaurus.io.
• I have read the console error message carefully (if applicable).

Description

With https://github.com/facebook/docusaurus/blob/main/packages/docusaurus-theme-classic/src/theme/MDXComponents/Ul/index.tsx

We have a few <ul>'s on our website, in an MDX file, and this script seems to be adding an empty class attribute to them, which breaks our CSS ul:not([class]).

Thanks to homotechsual, the fix is:

 export default function MDXUl(props: Props): JSX.Element {
-  return <ul {...props} className={transformUlClassName(props.className)} />;
+  return <ul {...props} className={props.className ? transformUlClassName(props.className) : null} />;
 }

Reproducible demo

N/A

Steps to reproduce

1. Create a something.mdx file:

• Example

2. View the DOM in the web browser, and the <ul> is now <ul class="">.

Expected behavior

The class attribute should not be added.

Actual behavior

The class attribute is added, as an empty string.

Your environment

• Public source code: N/A
• Public site URL: N/A
• Docusaurus version used: 2.4.0
• Environment name and version: Chrome 114.0.5735.134, Node.js 18.16.1
• Operating system and version: Windows 10 Enterprise

Self-service

• I'd be willing to fix this bug myself.

[slorber]

Thanks.

This looks like a good fix, feel free (or anyone else) to submit a PR and we'll merge it

---

Edit: PR is already there #9099

[thedevwonder]

@slorber I think this solves the issue for the MDXUl component but will create the same problem on other custom components for ex: MDXImg. @VanTsang888 is getting an empty className instead of no property at all, and I think this is why it fails at ul:not([class]).
The root: clsx package will give an empty string on undefined or null input values.

Solution 1:

I feel there should be a common function to transform className, that will handle our base condition. And then we can use the function for other components as well.

function transformElement(className?: string): string | undefined {
  if(className) return undefined;
  return clsx(
    className,
    //any additional name
  );
}  

Solution 2:

We can make the change for the other custom components as well in their implementation for now, instead of creating a common function. Just to make sure we are using this behavior only when needed.
Just like the above-mentioned solution here.

@slorber Can you please suggest which approach to take here? I can create a PR after that. I am new to the community so I'm still getting used to around here.

[thedevwonder]

I have created a PR for Solution 2 #9109. Tested it my local env, will need some help writing tests for the same.

@slorber I think this solves the issue for the MDXUl component but will create the same problem on other custom components for ex: MDXImg. @VanTsang888 is getting an empty className instead of no property at all, and I think this is why it fails at ul:not([class]). The root: clsx package will give an empty string on undefined or null input values.

Solution 1:

I feel there should be a common function to transform className, that will handle our base condition. And then we can use the function for other components as well.

function transformElement(className?: string): string | undefined {
  if(className) return undefined;
  return clsx(
    className,
    //any additional name
  );
}  

Solution 2:

We can make the change for the other custom components as well in their implementation for now, instead of creating a common function. Just to make sure we are using this behavior only when needed. Just like the above-mentioned solution here.

@slorber Can you please suggest which approach to take here? I can create a PR after that. I am new to the community so I'm still getting used to around here.

[slorber]

@thedevwonder I don't agree that this change is needed in other places, because other places usually use clsx with fixed/static classname.

Those are different cases:

• Ul: clsx(className,transform(className)): can eventually return '': bad
• Img: clsx("static-img-className",className): will never return ''

[thedevwonder]

Yes, you're correct. clsx("static-img-className", className) will never return an empty className. I was looking for a more general scenario where some other element may replicate the Ul behavior going forward in case it gets its own CSS and implementation. I guess I'll be over-optimizing things unnecessarily. Thanks for the clarification.