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

Incompatibility with SVG icons #5324

Closed
sandrocsimas opened this issue Nov 15, 2023 · 22 comments · Fixed by #5325
Closed

Incompatibility with SVG icons #5324

sandrocsimas opened this issue Nov 15, 2023 · 22 comments · Fixed by #5325
Assignees
Labels
Typescript Issue or pull request is *only* related to TypeScript definition
Milestone

Comments

@sandrocsimas
Copy link

sandrocsimas commented Nov 15, 2023

Describe the bug

I'm trying to use the <Button /> component with an icon (Icon-only) from @tabler/icons-react, but primereact seems to not work properly SVG icons.

Examples like the following are not working:

<Button size="large" icon={(options): JSX.Element => <IconTrash {...options.iconProps} />} />
// I copied this SVG from your website's example.
<Button
  size="large"
  icon={(options): JSX.Element => (
    <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" {...options.iconProps}>
      <g id="chevron-down">
        <path d="M12,15.25a.74.74,0,0,1-.53-.22l-5-5A.75.75,0,0,1,7.53,9L12,13.44,16.47,9A.75.75,0,0,1,17.53,10l-5,5A.74.74,0,0,1,12,15.25Z" />
      </g>
    </svg>
  )}
/>

Here is the Typescript error for @tabler/icons-react:

Type '{ accept?: string | undefined; acceptCharset?: string | undefined; action?: string | undefined; allowFullScreen?: boolean | undefined; allowTransparency?: boolean | undefined; alt?: string | undefined; ... 379 more ...; key?: Key | ... 1 more ... | undefined; }' is not assignable to type 'TablerIconsProps'.
  Types of property 'ref' are incompatible.
    Type 'LegacyRef<HTMLElement | SVGElement> | undefined' is not assignable to type 'LegacyRef<SVGSVGElement> | undefined'.
      Type 'RefObject<HTMLElement | SVGElement>' is not assignable to type 'LegacyRef<SVGSVGElement> | undefined'.
        Type 'RefObject<HTMLElement | SVGElement>' is not assignable to type 'RefObject<SVGSVGElement>'.
          Type 'HTMLElement | SVGElement' is not assignable to type 'SVGSVGElement'.
            Type 'HTMLElement' is missing the following properties from type 'SVGSVGElement': currentScale, currentTranslate, height, width, and 53 more.

And there is the error for the <svg />:

Type '{ children: Element; accept?: string | undefined; acceptCharset?: string | undefined; action?: string | undefined; allowFullScreen?: boolean | undefined; allowTransparency?: boolean | undefined; ... 381 more ...; viewBox: string; }' is not assignable to type 'SVGProps<SVGSVGElement>'.
  Types of property 'ref' are incompatible.
    Type 'LegacyRef<HTMLElement | SVGElement> | undefined' is not assignable to type 'LegacyRef<SVGSVGElement> | undefined'.
      Type 'RefObject<HTMLElement | SVGElement>' is not assignable to type 'LegacyRef<SVGSVGElement> | undefined'.
        Type 'RefObject<HTMLElement | SVGElement>' is not assignable to type 'RefObject<SVGSVGElement>'.

Result:

image

Reproducer

No response

PrimeReact version

10.0.9

React version

18.x

Language

TypeScript

Build / Runtime

Vite

Browser(s)

Brave 1.160.110

Steps to reproduce the behavior

Just paste the code in your React + Typescript project.

Expected behavior

The icon to be displayed with the correct size.

@sandrocsimas sandrocsimas added the Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible label Nov 15, 2023
@melloware melloware added Typescript Issue or pull request is *only* related to TypeScript definition and removed Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible labels Nov 15, 2023
@melloware melloware self-assigned this Nov 15, 2023
@melloware melloware added this to the 10.0.10 milestone Nov 15, 2023
@melloware
Copy link
Member

It still works its just a TypeScript issue see my example: https://stackblitz.com/edit/react-vsrrdc?file=src%2FApp.tsx

@sandrocsimas
Copy link
Author

Is there a way to fix this TypeScript issue? It is not compiling.
It is also not working when you set size='large'.

Here is the updated version of your Stackblitz with the library included:
https://stackblitz.com/edit/react-vsrrdc-dvr8jp?file=src%2FApp.tsx,src%2Findex.tsx,src%2Fstyle.css

image

@melloware
Copy link
Member

Size="large" works fine for me: https://stackblitz.com/edit/react-vsrrdc?file=src%2FApp.tsx

@sandrocsimas
Copy link
Author

sandrocsimas commented Nov 15, 2023

You need to remove the label. It is an icon-only button.
Also, with the size="small", the icon gets too small.
Please check this one: https://stackblitz.com/edit/react-vsrrdc-mmes15?file=src%2FApp.tsx

@melloware
Copy link
Member

melloware commented Nov 15, 2023

Fixed again: https://stackblitz.com/edit/react-vsrrdc?file=src%2FApp.tsx

Its because SVG's are NOT fonts so size="large" increases the button padding and font. Your SVG has no idea about the size so i had to tweak the values of the SVG height, width, and viewbox.

@sandrocsimas
Copy link
Author

This is what I see from your last link (Chrome and Brave):
image
At least, changing the result type would fix the TypeScript problem.
Thanks!

@melloware
Copy link
Member

Sorru missed a CSS reset for p-button-lg: https://stackblitz.com/edit/react-vsrrdc?file=src%2Fstyle.css

@melloware
Copy link
Member

image

@sandrocsimas
Copy link
Author

@melloware Sorry to bother you again.
Just to let you know that your fix seems to not work. I'm testing by modifying the type definition manually with your code:
node_modules/primereact/utils/utils.d.ts > IconOptions

// Doesn't work
// iconProps: React.HTMLProps<HTMLElement | SVGElement>;

// Doesn't work
// iconProps: React.HTMLProps<HTMLElement> | React.SVGProps<SVGSVGElement>;

// Doesn't work
// iconProps: React.SVGProps<SVGSVGElement> | React.HTMLProps<HTMLElement>;

// It works, but this will break the API. Need to find an alternative.
iconProps: React.SVGProps<SVGSVGElement>;

@melloware
Copy link
Member

yeah this is defintiely correct React.SVGProps<SVGSVGElement> but it also has to support regular HTML elements like <i> or <span> as non SVG icons are rendered that way.

@melloware
Copy link
Member

Hmm everything I am reading says iconProps: React.HTMLProps<HTMLElement> | React.SVGProps<SVGSVGElement>; is the correct way to do it?

@sandrocsimas
Copy link
Author

This is throwing the following error:

Types of property 'ref' are incompatible.
      Type 'LegacyRef<HTMLElement> | undefined' is not assignable to type 'LegacyRef<SVGSVGElement> | undefined'.
        Type '(instance: HTMLElement | null) => void' is not assignable to type 'LegacyRef<SVGSVGElement> | undefined'.
          Type '(instance: HTMLElement | null) => void' is not assignable to type '(instance: SVGSVGElement | null) => void'.
            Types of parameters 'instance' and 'instance' are incompatible.
              Type 'SVGSVGElement | null' is not assignable to type 'HTMLElement | null'.
                Type 'SVGSVGElement' is missing the following properties from type 'HTMLElement': accessKey, accessKeyLabel, autocapitalize, dir, and 24 more.

You could create a new property exclusively for SVGs, something like svgIconProps.

iconProps: React.HTMLProps<HTMLElement>;
svgIconProps: React.SVGProps<SVGSVGElement>;

@melloware
Copy link
Member

Can you try this...

iconProps: Omit<React.HTMLProps<HTMLElement>, 'ref'> | Omit<React.SVGProps<SVGSVGElement>, 'ref'>;

@sandrocsimas
Copy link
Author

sandrocsimas commented Nov 15, 2023

It does not work. There are many other conflicting properties:

<Button icon={(options): JSX.Element => <IconTrash {...options.iconProps} />} />

Type '{ disabled?: boolean | undefined; key?: Key | null | undefined; form?: string | undefined; formAction?: string | undefined; formEncType?: string | undefined; formMethod?: string | undefined; ... 378 more ...; wrap?: string | undefined; } | { ...; }' is not assignable to type 'IntrinsicAttributes & TablerIconsProps'.
  Type '{ disabled?: boolean | undefined; key?: Key | null | undefined; form?: string | undefined; formAction?: string | undefined; formEncType?: string | undefined; formMethod?: string | undefined; ... 378 more ...; wrap?: string | undefined; }' is not assignable to type 'TablerIconsProps'.
    Types of property 'onCopy' are incompatible.
      Type 'ClipboardEventHandler<HTMLElement> | undefined' is not assignable to type 'ClipboardEventHandler<SVGSVGElement> | undefined'.
        Type 'ClipboardEventHandler<HTMLElement>' is not assignable to type 'ClipboardEventHandler<SVGSVGElement>'.
          Type 'HTMLElement' is missing the following properties from type 'SVGSVGElement': currentScale, currentTranslate, height, width, and 53 more.

@melloware
Copy link
Member

Yeah for some reason it doesn't like the Either OR.

@sandrocsimas
Copy link
Author

Yeah, that's weird. =(

@melloware
Copy link
Member

One more try this?

iconProps: React.HTMLProps<Element>;

its super generic.

@melloware
Copy link
Member

And also..

iconProps: React.HTMLProps<any> | React.SVGProps<any>;

@sandrocsimas
Copy link
Author

Not yet:

Type '{ accept?: string | undefined; acceptCharset?: string | undefined; action?: string | undefined; allowFullScreen?: boolean | undefined; allowTransparency?: boolean | undefined; alt?: string | undefined; ... 379 more ...; key?: Key | ... 1 more ... | undefined; }' is not assignable to type 'TablerIconsProps'.
  Types of property 'ref' are incompatible.
    Type 'LegacyRef<Element> | undefined' is not assignable to type 'LegacyRef<SVGSVGElement> | undefined'.
      Type 'RefObject<Element>' is not assignable to type 'LegacyRef<SVGSVGElement> | undefined'.
        Type 'RefObject<Element>' is not assignable to type 'RefObject<SVGSVGElement>'.
          Type 'Element' is missing the following properties from type 'SVGSVGElement': currentScale, currentTranslate, height, width, and 151 more.

@sandrocsimas
Copy link
Author

iconProps: React.HTMLProps<any> | React.SVGProps<any>; Works

@melloware
Copy link
Member

OK I committed that change.

@sandrocsimas
Copy link
Author

I mean, we still have integration problems between primereact and SVG icon libraries, but at least it compiles, hehe.
Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Typescript Issue or pull request is *only* related to TypeScript definition
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants