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

Add id to TS file #326

Merged
merged 2 commits into from Mar 5, 2020
Merged

Add id to TS file #326

merged 2 commits into from Mar 5, 2020

Conversation

rodlukas
Copy link

@rodlukas rodlukas commented Mar 4, 2020

corresponding issue: #324

@robmadole
Copy link
Member

Thanks @rodlukas. That will definitely do it but is that just delaying another issue with a different property?

My question is what does this component need to extend to include this automatically?

For example this line in the type definitions mentions the id?: def.

Should the interface be changed to include HTMLAttributes?

export interface FontAwesomeIconProps extends BackwardCompatibleOmit<DOMAttributes<SVGSVGElement>, 'children' | 'mask'> {

@rodlukas
Copy link
Author

rodlukas commented Mar 4, 2020

@robmadole Yeah, this commit only solves the id attr, another attributes that someone could need in the future would not work again and again.

The interface can be IMO changed without any conflicts to HTMLAttributes:

export interface FontAwesomeIconProps extends BackwardCompatibleOmit<HTMLAttributes<SVGSVGElement>, 'children' | 'mask'> {

OR in the file there's also SVGAttributes interface, which is closer to the output here (svg) and thus probably better, but there would be one property in conflict - transform. Here in FA it has transform?: string | Transform, but SVGAttributes defines it like transform?: string; - these defs are not compatible. It can be solved by omitting this transform property:

export interface FontAwesomeIconProps extends BackwardCompatibleOmit<SVGAttributes<SVGSVGElement>, 'children' | 'mask' | 'transform'> {

@robmadole
Copy link
Member

robmadole commented Mar 5, 2020

Thank you @rodlukas !

I think this one is the winner:

export interface FontAwesomeIconProps extends BackwardCompatibleOmit<SVGAttributes<SVGSVGElement>, 'children' | 'mask' | 'transform'> {

Is there a way where you could give that a quick smoke-test locally and make sure it doesn't immediately cause issues?

@rodlukas
Copy link
Author

rodlukas commented Mar 5, 2020

@robmadole Successfully tested on my local larger codebase, everything works, TS compiler is OK, ESLint is OK even with id attributes on FA. Icons are successfully rendered. LGTM.

Here's the index.d.ts code I tested (note the import section must be also updated)
 /// <reference types="react" />
 import { CSSProperties, SVGAttributes } from 'react'
 import {
   Transform,
   IconProp,
   FlipProp,
   SizeProp,
   PullProp,
   RotateProp,
   FaSymbol
 } from '@fortawesome/fontawesome-svg-core'

 export function FontAwesomeIcon(props: FontAwesomeIconProps): JSX.Element

 /**
  * @deprecated use FontAwesomeIconProps
  */
 export type Props = FontAwesomeIconProps

 // This is identical to the version of Omit in Typescript 3.5. It is included for compatibility with older versions of Typescript.
 type BackwardCompatibleOmit<T, K extends keyof any> = Pick<T, Exclude<keyof T, K>>;

 export interface FontAwesomeIconProps extends BackwardCompatibleOmit<SVGAttributes<SVGSVGElement>, 'children' | 'mask' | 'transform'> {
   icon: IconProp
   mask?: IconProp
   className?: string
   color?: string
   spin?: boolean
   pulse?: boolean
   border?: boolean
   fixedWidth?: boolean
   inverse?: boolean
   listItem?: boolean
   flip?: FlipProp
   size?: SizeProp
   pull?: PullProp
   rotation?: RotateProp
   transform?: string | Transform
   symbol?: FaSymbol
   style?: CSSProperties
   tabIndex?: number;
   title?: string;
   swapOpacity?: boolean;
 }

@robmadole
Copy link
Member

I'm happy enough with your result to go ahead and publish it. If we have issues we can do another release.

@rodlukas are you willing to update this PR with your tested changes?

@robmadole robmadole merged commit 4b49f54 into FortAwesome:master Mar 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants