[go: up one dir, main page]

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

Tailwind classes do not work as prop of component #1250

Closed
arsistyle opened this issue Jan 25, 2024 · 16 comments
Closed

Tailwind classes do not work as prop of component #1250

arsistyle opened this issue Jan 25, 2024 · 16 comments

Comments

@arsistyle
Copy link

Describe the Bug

I am creating some small components to which other tailwind classes can be added as porps, but they are not being visible, but if I passed any other word as a class it works.

Here is a little demo:

with Tailwind classes

import React from 'react'
import { Text, Container } from '@react-email/components'

import { Main } from '../src/layout/CleanLayout'

const ComponentX = ({className}) => {
  return <p className={`mb-8 text-red-500 ${className}`}>Im a component</p>
}

export default function Email() {
  return (
    <Main>
      <Container className="px-4">
        <Text className="text-3xl">Hello world</Text>
        <ComponentX className="text-blue-500 bg-black" />
      </Container>
    </Main>
  )
}

and this is the output of the component:

<p class="undefined" style="margin-bottom:32px;color:rgb(239,68,68)">Im a component</p>

with Tailwind classes and random words

import React from 'react'
import { Text, Container } from '@react-email/components'

import { Main } from '../src/layout/CleanLayout'

const ComponentX = ({className}) => {
  return <p className={`mb-8 text-red-500 ${className}`}>Im a component</p>
}

export default function Email() {
  return (
    <Main>
      <Container className="px-4">
        <Text className="text-3xl">Hello world</Text>
        <ComponentX className="text-blue-500 bg-black foo bar" />
      </Container>
    </Main>
  )
}

and this is the output of the component:

<p class="foo bar" style="margin-bottom:32px;color:rgb(239,68,68)">Im a component</p>

Which package is affected (leave empty if unsure)

@react-email/tailwind

Link to the code that reproduces this issue

i dont have

To Reproduce

Just create a simple component like example on description

Expected Behavior

I expect that tailwind classes can be passed as properties of a component.

What's your node version? (if relevant)

20.11.0

@arsistyle arsistyle added the Type: Bug Confirmed bug label Jan 25, 2024
@gabrielmfern
Copy link
Collaborator

I think this might be happening because they are being turned into styles to your component and passing them as props.
Try taking a style prop into the underlying element for ComponentX to see what happens.

I also saw something related to this when someone was asking about how they could use tailwind-merge with our Tailwnd component. This is certainly a bug that causes unexpected behavior, some case we are missing to add treatment for, but these kinds of experiments might reveal a bit of insight into what is happening here.

@arsistyle
Copy link
Author

this way?

because i have the same behavior

const ComponentX = ({ className }) => {
  return (
    <p className={`mb-8 text-red-500`}>
      <span className={className}>Im a component</span>
    </p>
  )
}

export default function Email() {
  return (
    <Main>
      <Container className="px-4">
        <Text className="text-3xl">Hello world</Text>
        <ComponentX className="text-blue-500 bg-black foo bar" />
      </Container>
    </Main>
  )
}
<p style="margin-bottom:32px;color:rgb(239,68,68)">
  <span class="foo bar">Im a component</span>
</p>

@gabrielmfern
Copy link
Collaborator

I mean like this

const ComponentX = ({ className, style }) => {
  return (
    <p className={`mb-8 text-red-500 ${className}`} style={style}>
      Im a component
    </p>
  )
}

export default function Email() {
  return (
    <Main>
      <Container className="px-4">
        <Text className="text-3xl">Hello world</Text>
        <ComponentX className="text-blue-500 bg-black foo bar" />
      </Container>
    </Main>
  )
}

@arsistyle
Copy link
Author

Well, it works only if the added class does not exist before, see class text.

const ComponentX = ({ className, style }: { className?: string; style?: any }) => {
  return (
    <p className={`mb-8 text-red-500 ${className}`} style={style}>
      Im a component
    </p>
  )
}

export default function Email() {
  return (
    <Main>
      <Container className="px-4">
        <Text className="text-3xl">Hello world</Text>
        <ComponentX className="text-blue-500 bg-black foo bar" />
      </Container>
    </Main>
  )
}
<p class="foo bar" style="color:rgb(239,68,68);background-color:rgb(0,0,0);margin-bottom:32px">Im a component</p>

the class text-blue-500 does not apply because text-red-500 exists.

image

@gabrielmfern
Copy link
Collaborator
gabrielmfern commented Jan 25, 2024

Ok, so this seems like another issue, then two things break this:

  1. Components without a style prop that have classNames do not work properly
  2. Styles from class names do not take priority based on the same order Tailwind does

I think your second issue there also would be helped a lot by being able to use tailwind merge here.

@arsistyle
Copy link
Author

Well yes, tailwind-merge solved my second problem, thanks for that suggestion.

About the first problem, it's a bit tedious, but it's not something that keeps me awake at night, so I'll wait quietly if they can fix this bug.

Thanks for the help!

@gabrielmfern
Copy link
Collaborator
gabrielmfern commented Jan 26, 2024

What we currently do on the Tailwind component is do a pass over the JSX tree
while inlining styles for everything, including components.

This is probably what is generally harmful here, maybe what we should do instead is inline styles
for only elements and then, for components define a wrapper component around them that
uses Tailwind on its resolved children only.

Lmk if this makes sense in your situation here, tailwind merge's behavior is also something to consider
but I also think this should get that up and running as expected as well.

@gabrielmfern
Copy link
Collaborator

Waiting on #1124 to be merged before working on this.

@kotAPI
Copy link
kotAPI commented Apr 15, 2024

I have the same issue, tailwind color classes dont apply when above pattern is followed. Its becoming very difficult to build components that can be reused with this bug around.

@kotAPI
Copy link
kotAPI commented Apr 15, 2024

Weirdly, for me this somehow worked - for my Text wrapper component

  • I had to remove all the inline styles
  • passed the ...props to the Text component
const Text = ({ underline, className = "", italic, bold, children, ...props }) => {
    var finalClasses = twMerge(className, underline ? 'underline' : '', italic ? 'italic' : '', bold ? 'font-bold' : '', commonStyleClasses, "inline")

    return <Text className={finalClasses}
        {...props}
    >{children}</Text>
}

I have no idea how or why this works, would be great if you can shed some light, as its very counter intuitive and is very different from how developers usually build components

@WillsB3
Copy link
WillsB3 commented May 1, 2024

I'm also seeing this.

If i make a component like this:

export const Paragraph = ({
  children,
  className,
  style,
  strong,
  ...props
}: ParagraphProps) => {
  const classes = cn(
    'text-black text-m font-normal not-italic leading-normal',
    { 'font-bold': strong },
    className
  )

  if (props.test) {
    console.log('[P] classes', classes)
  }

  return (
    <Text className={classes} style={style} {...props}>
      {children}
    </Text>
  )
}

and then use it like this:

<P strong test="true" className="font-semibold">
  If you need to reschedule or cancel your call
</P>

The rendered paragraph has font-weight 700 instead of 600 and I see this in the terminal:

[P] classes text-m not-italic leading-normal font-semibold
[P] classes text-m not-italic leading-normal font-bold

So it looks like the first render(?) has the correct class, but the second does not…

@kotAPI
Copy link
kotAPI commented May 2, 2024

I'm also seeing this.

If i make a component like this:

export const Paragraph = ({
  children,
  className,
  style,
  strong,
  ...props
}: ParagraphProps) => {
  const classes = cn(
    'text-black text-m font-normal not-italic leading-normal',
    { 'font-bold': strong },
    className
  )

  if (props.test) {
    console.log('[P] classes', classes)
  }

  return (
    <Text className={classes} style={style} {...props}>
      {children}
    </Text>
  )
}

and then use it like this:

<P strong test="true" className="font-semibold">
  If you need to reschedule or cancel your call
</P>

The rendered paragraph has font-weight 700 instead of 600 and I see this in the terminal:

[P] classes text-m not-italic leading-normal font-semibold
[P] classes text-m not-italic leading-normal font-bold

So it looks like the first render(?) has the correct class, but the second does not…

yup, same behaviour - kind of goes against the purpose of building your own components, hope this gets fixed sometime soon

@WillsB3
Copy link
WillsB3 commented May 10, 2024

@gabrielmfern Am I right in thinking that we need #1383 merged before this issue can be fixed? Do you think any extra changes will be required on top of those included in #1383 to fix the problem outlined in this issue?

@gabrielmfern
Copy link
Collaborator

@WillsB3 I actually have a fix for this on a branch, but there's no PR for it yet. It requires #1383 to be merged for it to work properly.

@kotAPI
Copy link
kotAPI commented Jun 4, 2024

#1383
Any updates on this or a temporary workaround?

@gabrielmfern
Copy link
Collaborator

Hey everyone, I was able to find a way to fix this without #1383 and it works so much better than before. I have just released this on @react-email/tailwind@0.1.0-canary.1 or @react-email/components@0.0.23-canary.2.

Let me know if it works on the new version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants