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

Missing id attribute in typescript declarations #324

Closed
rodlukas opened this issue Feb 23, 2020 · 6 comments · Fixed by equinor/radix-web-console#195 · May be fixed by armando-andre/gatsby-personal-portfolio#4
Closed

Comments

@rodlukas
Copy link

Describe the bug
Typescript interface FontAwesomeIconProps does not include id attribute. Althought the id attribute is correctly emmited by FontAwesomeIcon in real when used.

Reproducible test case
Not neccessary - open file https://github.com/FortAwesome/react-fontawesome/blob/master/index.d.ts, interface FontAwesomeIconProps.

Expected behavior
Typescript interface FontAwesomeIconProps should include the id attribute:

id?: string
@robmadole
Copy link
Member

@rodlukas I don't think that this component is emitting id. Can you provide a reproducible example so we can see what's going on? (Also the FontAwesomeIconProps are the input to the component–not the output)

@robmadole
Copy link
Member

I'll re-open this one if I'm missing the point of your ticket.

@rodlukas
Copy link
Author

rodlukas commented Feb 27, 2020

@robmadole here's the sandbox: https://codesandbox.io/s/intelligent-murdock-o2bt9?eslint=1&fontsize=14&hidenavigation=1&theme=dark

id prop is set on FontAwesomeIcon to testID, this value is then emitted to the svg element in the DOM, but as you can see, the linter is yelling about missing id attribute in FontAwesomeIconProps.

The only workaround is wrapping the FontAwesomeIcon with span with id, but only because of the linter, because the id is already there even without this workaround.

2020-02-27 (1)

@robmadole
Copy link
Member

Gotcha. Here is the TypeScript definition file. We don't use TS so I have no idea how it should be changed to support the id property. Can you recommend a change?

@robmadole robmadole reopened this Mar 4, 2020
@rodlukas
Copy link
Author

rodlukas commented Mar 4, 2020

@robmadole here's the PR - #326.
The id attribute is practically the same as ie. className attribute in the TS file.

@robmadole
Copy link
Member

PR merged. Thanks a bunch @rodlukas !

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