tremorlabs / tremor

React components to build charts and dashboards
https://tremor.so
Apache License 2.0
15.53k stars 450 forks source link

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

Closed jschuur closed 1 week ago

jschuur commented 3 months ago

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 commented 3 months ago

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

BenJenkinson commented 2 months ago

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.

https://github.com/tremorlabs/tremor/blob/bd6566b814d907e130b6827194def10b5bc3661f/src/components/vis-elements/BarList/BarList.tsx#L101-L113

severinlandolt commented 2 months ago

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 commented 2 months ago

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.

https://github.com/tremorlabs/tremor/blob/bd6566b814d907e130b6827194def10b5bc3661f/src/components/vis-elements/BarList/BarList.tsx#L132-L134

It's currently typed as name: string

https://github.com/tremorlabs/tremor/blob/bd6566b814d907e130b6827194def10b5bc3661f/src/components/vis-elements/BarList/BarList.tsx#L17

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

https://github.com/tremorlabs/tremor/blob/bd6566b814d907e130b6827194def10b5bc3661f/src/components/vis-elements/BarList/BarList.tsx#L75

severinlandolt commented 2 months ago

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

github-actions[bot] commented 1 week ago

:tada: This issue has been resolved in version 3.17.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: