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 "title" prop to set/overwrite SVG title #18

Merged
merged 3 commits into from
Jul 24, 2020

Conversation

mjr9804
Copy link
Contributor

@mjr9804 mjr9804 commented Jun 29, 2020

Adds a title prop to allow setting the content of the SVG's <title> element. If the SVG already has a title, it will be overwritten with the value of the prop. Otherwise, a <title> is created and appended to the SVG.

Fixes #3

@shrpne
Copy link
Owner

shrpne commented Jul 1, 2020

Thank you for the PR! I will try to take a look at the weekend

@mjr9804
Copy link
Contributor Author

mjr9804 commented Jul 8, 2020

I ran into one issue with this change. Since the title is set when the SVG is downloaded, you can't use the same image in 2 places with different titles. The first title that is set will always be used since the image is (nicely) only downloaded once and then cached.

I'll look at moving the setTitle to a more appropriate place when I have some free time.

@mjr9804
Copy link
Contributor Author

mjr9804 commented Jul 14, 2020

Updated with a better diff that sets the title each time the image is used (not just when the SVG is downloaded)

@shrpne
Copy link
Owner

shrpne commented Jul 14, 2020

Nice! I hope I will have some free time this week to review and publish it.

@shrpne shrpne merged commit bbc6d03 into shrpne:master Jul 24, 2020
setTitle(svg) {
const titleTags = svg.getElementsByTagName('title');
if (titleTags.length) { // overwrite existing title
titleTags[0].innerHTML = this.title;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking of replacing .innerHTML with .innerText.
It will prevent XSS.
On the other hand, it will disable inserting other tags or html entities, like &nbsp;. But titles are used for accessibility and screen readers. Usually they doesn't need these features.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh, good point. I'd go with .innerText instead.

@shrpne
Copy link
Owner

shrpne commented Jul 28, 2020

Released under v2.0.0. Thank you again!

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.

Set (overwrite) title
2 participants