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

How to append a title element to the SVG ReactComponent #7103

Closed
sudkumar opened this issue May 24, 2019 · 3 comments · Fixed by #7118
Closed

How to append a title element to the SVG ReactComponent #7103

sudkumar opened this issue May 24, 2019 · 3 comments · Fixed by #7118
Assignees

Comments

@sudkumar
Copy link
Contributor

What I wanted

Given a phone.svg file (which has a title element as one of its children), I want to render it with a custom title based on a prop. Currently, passing a title prop to the ReactComponent (import { ReactComponent } from 'phone.svg') doesn't update the content of the title element inside rendered svg.

What I tried

As described in svgr docs, we can use a titleProp option in our webpack config to append a title element to the rendered svg element. But in our webpack.config.js file, we are only passing +ref option to the @svgr/webpack loader which doesn't append the title element.

So, just for testing purpose, I changed to webpack.config.js to include the titleProp option in the loader as follows

{
  svg: {
-    ReactComponent: '@svgr/webpack?-svgo,+ref![path]',
+    ReactComponent: '@svgr/webpack?-svgo,+titleProp,+ref![path]',
  },
}

After this, if I pass a title prop to the Svg component, it was appending a title element to the svg element, which is what I wanted.

import { ReactComponent as Phone } from "./svgs/phone.svg"

function CallTo ({ name, phoneNumber }) {
  return <Phone title={`Call to ${name} on ${phoneNumber}`} />
}
.
.
<CallTo name="React" phoneNumber="123123" />
<svg ...>
<title>Call to React on 123123</title>
.
</svg>

This is great as now I can pass any context based title for an Icon which will make it more accessible than just a static default title for the icon. I can make a PR to update the configuration for webpack.

Where is the problem then

The problem with updating the webpack configuration is that If I don't pass a title to the SVG component, it will render an empty title even if the svg file has a title element (e.g. Phone), which is not a desired default functionality.

What is the solution

I don't know, yet. May be, @svgr/babel-plugin-svg-dynamic-title need to be updated first to handle the case where the title props in null (i.e. it should not update the default title when the title prop is null). And after that, we can make a change to the webpack configuration file. But I don't know where to start.

What I am doing currently

I am using aria-label attribute to make the Svg React Component. And I have also removed the title element from my svg files. Removal of title is required because hovering-over to the svg reveals the title element's content (Phone) which stays the same for all the instances of this icon on a page and svg element doesn't accept a title prop.

@mrmckeb
Copy link
Contributor

mrmckeb commented May 27, 2019

Hi @sudkumar, thanks for raising this.

I think we can accept a PR that solves this. Can you put this together and assign it to me? Thanks!

@mrmckeb mrmckeb self-assigned this May 27, 2019
@sudkumar
Copy link
Contributor Author

Hi @mrmckeb . Thank you for having a look into it.

I dig into it and found that the issue is with the svgr itself. I am waiting for a PR (gregberge/svgr#311) to get merged. This PR solves the issue that we will not affect any existing code base using SVG ReactComponent when we updated our webpack.config.js file.

{
  svg: {
-    ReactComponent: '@svgr/webpack?-svgo,+ref![path]',
+    ReactComponent: '@svgr/webpack?-svgo,+titleProp,+ref![path]',
  },
}

If I make this change here before that PR (gregberge/svgr#311) merges, we may have an issue that the title element will get removed as this is the current behaviour of SVGR with titleProp set to true.

I will open the PR as soon as the other PR gets merged.

Please suggest how I should proceed.

@mrmckeb
Copy link
Contributor

mrmckeb commented May 29, 2019

Awesome, thank you! I'll close this off for now, but if you do raise a PR, assign to me and link to this ticket. Thanks!

@mrmckeb mrmckeb closed this as completed May 29, 2019
@lock lock bot locked and limited conversation to collaborators Jun 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants