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

data-fa-title-id prop is not working on FortAwesomeIcon #337

Closed
KKS1 opened this issue Apr 23, 2020 · 7 comments · Fixed by #397
Closed

data-fa-title-id prop is not working on FortAwesomeIcon #337

KKS1 opened this issue Apr 23, 2020 · 7 comments · Fixed by #397

Comments

@KKS1
Copy link

KKS1 commented Apr 23, 2020

Describe the bug
As mentioned in https://fontawesome.com/how-to-use/on-the-web/other-topics/accessibility, if we provide title and want to have snapshot testing on a component, 'data-fa-title-id' keeps it unique, instead of adding random Ids.
But this property is not working when specified on React FontAwesomeIcon component.

Reproducible test case
https://codesandbox.io/s/weathered-microservice-4g67n?file=/src/App.js

Expected behavior
Passing data-fa-title-id should keep the Id on title element in SVG, to remain unique and based on this passed prop; instead of random values.

Desktop (please complete the following information):

  • Browser [Chrome]
  • Version [80.0.3987.87]
@robmadole
Copy link
Member

We haven't updated this component to support the new title id prop. It's on the todo list and PRs are welcome!

@xiao-hu
Copy link

xiao-hu commented Jun 7, 2020

When I tried to do SSR, the client will generate a different id than the server does, resulting in a warning "Prop id did not match". I think we'll also need an option to disable title id all togather.

@robmadole
Copy link
Member

@xiao-hu the idea is to explicitly set it so that snapshot tests (and SSR) have a predictable value.

Why would we also need the ability to disable it? That compromises the accessibility of it.

@blopa
Copy link

blopa commented Jan 8, 2021

I still have this problem when building using SSR and Webpack 5. It works fine on Webpack 4.

Edit: I just checked and it actually didn't work on Webpack 4 too 😅

If I use the new titleId prop, then I get a different warning, saying that the titleId is null in the server, and it's my-id in the client. I also get an error saying that titleId is not a valid prop for FontAwesomeIcon, but I guess this is because it's missing it in the FontAwesomeIconProps? Anyway I made a PR for it #406

@imbianchi
Copy link

Hello everyone (:
So, I'm having the same issue on my project. The opened PR #406 by @blopa will be merged?

@FelixButzbach
Copy link

Hei there.
We also experience the exact same problem as @blopa . Any plans on merging his PR? Or is there another workaround?

@tswaters
Copy link

tswaters commented Jun 4, 2021

The linked PR only applies titleId to the type definitions.

If titleId: '' is added to the defaultProps, around here:

, the code that applies extra props here:
extraProps[key] = props[key]
picks it up and the react warning seems to goes away?

React does not recognize the titleId prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase titleid instead. If you accidentally passed it from a parent component, remove it from the DOM element.
in svg (created by FontAwesomeIcon)

I'm not sure it makes sense to provide a default value for this option though. In the convert function it might make more to improve the if condition here -- for attributes passed here that are not valid dom attributes that react will complain about (i.e., titleId -- not sure if there are others?) just ignore them when converting it into a react node.... here:

if (key.indexOf('aria-') === 0 || key.indexOf('data-') === 0) {
acc.attrs[key.toLowerCase()] = val
} else {
acc.attrs[camelize(key)] = val
}

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 a pull request may close this issue.

7 participants