-
-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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
Append title element to SVG component via title prop #7118
Conversation
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
@mrmckeb Here I made the attempt to fix it. |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
@mrmckeb gregberge/svgr#311 is merged now. Waiting for a new release of @svgr/webpack then I will update the package here too. |
…the titleProp update for fallback title
New version of Tested it locally and got an issue (gregberge/svgr#314) which causes this to not work in CRA. Opened a PR (gregberge/svgr#315) to fix this. Will update once resolves. |
Thanks for the update, keep us posted! |
The PR (gregberge/svgr#315) is merged. Waiting for a new release. |
@mrmckeb PR is ready for review. Please have a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thanks. I see some template changes snuck into the PR too - which I think are fine.
@sudkumar Thanks for the PR. it might be worth adding this to the docs - https://facebook.github.io/create-react-app/docs/adding-images-fonts-and-files#adding-svgs What do you think? |
@mrmckeb Thank you. And for docs: Yes. I will make a PR to update the docs as well. |
do you think you could provide an approx date for this to be released? Thanks |
This will be in the next release which we're currently working on, I think in the next two weeks or so. I'll confirm with @iansu on Wednesday :) |
Thank you. |
Fixes #7103 Add title to SVG component.
Native svg element doesn't accept a title prop. It requires a
<title>
children. Currently in CRA SVG component, we don't have an option to add a title for our SVG components. This PR enable us to add a title element by providing atitle
prop to SVG components.renders
We are using svgr's titleProp option to inject a title element for our SVG components. If no title is provided to the SVG component at render time, this SHOULD fallback to svg's title element.
I have tested it on my local machine by creating a new application with
yarn create-react-app my-app
(after reading contribution instructions) and it is working as expected forlogo.svg
file in our template.Why WIP
We are waiting for gregberge/svgr#311 to get merge to avoid affecting existing code bases which might have title element inside there svg files because, the current behaviour of svgr's
titleProp
option is to render/replace any title in an svg with an element that only renders the title provided by the prop.For example: Given our svg file
if we set
titleProp
to true, it will return an SVG component asWhich is not what we want because it will not fallback to the default title for svg. What we want is, when the above svg file gets processed by svgr, the output should be:
And so we are waiting for that PR (on svgr) to get merged before we proceed on this PR.