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

[Feature]: doc layout components should merge className #1612

Open
ap0nia opened this issue Nov 20, 2024 · 1 comment · May be fixed by #1618
Open

[Feature]: doc layout components should merge className #1612

ap0nia opened this issue Nov 20, 2024 · 1 comment · May be fixed by #1618

Comments

@ap0nia
Copy link
Contributor

ap0nia commented Nov 20, 2024

What problem does this feature solve?

Currently, all of the internal components used by the DocLayout will completely override the className property. For example, ul will always have "list-disc pl-5 my-4 leading-7". I think it should respect any className it gets from props, and attempt to merge them.

The motivation for this is that I wrote a custom rehype plugin that adds custom classes to MDX elements, but these are stripped away by the components.

What does the proposed API look like?

  1. The simplest solution would be to concatenate the props.
export const Ul = (props: ComponentProps<'ul'>) => {
  return <ul {...props} className={`list-disc pl-5 my-4 leading-7 ${props.className}`} />;
};

However, this may add an extra space if there were no classes provided.

  1. A nice solution would be to use the common cn utility for merging TailwindCSS classes.
import { clsx, type ClassValue } from "clsx"
import { twMerge } from "tailwind-merge"

export function cn(...inputs: ClassValue[]) {
  return twMerge(clsx(inputs))
}

export const Ul = (props: ComponentProps<'ul'>) => {
  return <ul {...props} className={cn("list-disc pl-5 my-4 leading-7", props.className)} />;
};

This would require additional packages to be installed, but they are small and might be useful in other places.

  1. Finally, it may also be feasible to implement an internal implementation of cn to prevent additional dependencies.
export function cn(...inputs: unknown[]) {
  return inputs.filter(Boolean).join(' ')
}

export const Ul = (props: ComponentProps<'ul'>) => {
  return <ul {...props} className={cn("list-disc pl-5 my-4 leading-7", props.className)} />;
};

If any of these solutions seem acceptable, I can open a corresponding PR.

@Timeless0911
Copy link
Collaborator

Timeless0911 commented Nov 20, 2024

The built-in TailwindCSS has caused some issues like style pollution since there are no prefix in built-in tailwind css classnames, we may change to use css modules or adding prefix for built-in tailwind classnames which has not been decided yet.

Therefore, I think option 1 or 3 is better, and below shows we mix the use of tailwindcss and css modules 😂

export const Blockquote = (props: ComponentProps<'blockquote'>) => {
  return (
    <blockquote
      {...props}
      className={`border-l-2 border-solid border-divider pl-4 my-6 transition-colors duration-500 ${styles.blockquote}`}
    />
  );
};

You can implement a function that judge if there were no classes provided. If provided, do concat. If not, return do nothing.

@ap0nia ap0nia linked a pull request Nov 21, 2024 that will close this issue
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants