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

Bug: BarList takes data with JSX Elements, but shows type error #945

Closed
jschuur opened this issue Feb 5, 2024 · 6 comments · Fixed by #1050
Closed

Bug: BarList takes data with JSX Elements, but shows type error #945

jschuur opened this issue Feb 5, 2024 · 6 comments · Fixed by #1050
Labels

Comments

@jschuur
Copy link

jschuur commented Feb 5, 2024

Tremor Version

3.13.4

Link to minimal reproduction

n/a

Steps to reproduce

I'm setting the name property of the data array passed into a BarList to a JSX Element and this renders fine, but I get a TypeScript error because the type for data doesn't allow for Elements.

  const listData = data
    ? orderBy(
        data.map((d) => ({
          name: <Badge color={resourceColor(d.resource_type)}>{d.resource}</Badge>,
          value: d.total_cost,
        })),
        'value',
        'desc'
      )
    : [];

CleanShot 2024-02-05 at 10 00 53@2x

What is expected?

Should not show TypeScript error if it can render JSX Elements in the data.

What is actually happening?

TypeScript error.

@severinlandolt
Copy link
Member

Hi @jschuur has this issue been resolved with v3.14?

@BenJenkinson
Copy link

BenJenkinson commented Mar 23, 2024

Hi @severinlandolt, I can confirm it is React.JSXElementConstructor<any> in v3.14.0

Types of property 'icon' are incompatible.
Type 'Element' is not assignable to type 'JSXElementConstructor<any> | undefined'
// It's happy with this
✅ icon: Star
✅ icon: () => <Star />

// But not this:
❌ icon: <Star />

I'm assuming that it doesn't accept icon: <Star /> since it wants to apply CSS classes.

{Icon ? (
<Icon
className={tremorTwMerge(
makeBarListClassName("barIcon"),
// common
"flex-none h-5 w-5 mr-2",
// light
"text-tremor-content",
// dark
"dark:text-dark-tremor-content",
)}
/>
) : null}

@severinlandolt
Copy link
Member

Hey there! This is a use-case I haven't thought of. Thanks @BenJenkinson for investigating. Since we apply our own className, I am not keen on changing this logic at the moment.

However: We recently launched Tremor Raw, a copy & paste library for React components. Here we have a new version of the BarList, where you have full control over this :)

@BenJenkinson
Copy link

Hi @severinlandolt & @jschuur,

My apologies, but I just realised that I misread @jschuur's question.

He wasn't asking about the icon, but the name property.

The name property is rendered as children, so it could easily be typed as a name: ReactNode instead.

>
{item.name}
</a>

It's currently typed as name: string

The only thing that would need to change is the two places where the name is used as a fallback for the key prop; can't do that if it is a ReactNode

key={item.key ?? item.name}

@severinlandolt
Copy link
Member

Just published an overhauled version of the BarList, happy to review a PR for this :)

Copy link

🎉 This issue has been resolved in version 3.17.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging a pull request may close this issue.

3 participants