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

Fix: Remove <title> tags from SVGs #207

Merged
merged 5 commits into from
May 26, 2017

Conversation

mladenplavsic
Copy link
Contributor

@mladenplavsic mladenplavsic commented May 20, 2017

As described in #205 <title> tags are removed in these usages of SVG:

  • svg-min/*.svg
  • php/gridicons.php
  • svg-sprite/gridicons.svg

This is done by updating Gruntfile.js - removed "addtitle" task.

Some more things done:

@folletto
Copy link
Contributor

includeTitleElement = false? Clever. :)

I'm a bit uncomfortable with fileContent.indexOf( '>' ) which seems too generic, but as we use GIT/PRs if there are odd results should be visible. For reference, I already raised the point in the past, so it's just to make sure we're aware, and there's an issue to fix it: #123.


To test, I checked:

  1. No more title in svg-min/*.svg ✅
  2. No more title in php/gridicons.php ✅
  3. No more title in svg-sprite/gridicons.svg ✅
  4. Run grunt, no differences expected ✅

Note: I had to run npm install before running grunt for this to work properly.

@folletto
Copy link
Contributor

@bluefuton can you drop a quick eye here on the code to have a more expert review? Should be quick :)

@davewhitley
Copy link
Contributor

davewhitley commented May 24, 2017

My PR #208 caused the conflicts. Looks like this PR collapses the sprite into one line. Note that I added a new icon in #208

@davewhitley davewhitley requested a review from bluefuton May 24, 2017 21:09
@bluefuton
Copy link
Contributor

Thanks @mladenplavsic! Code looks good to me - just needs rebasing and SVGs regenerating I think 👍

Conflicts:
	docs/gridicons.svg
	svg-sprite/gridicons.svg
Copy link
Contributor

@bluefuton bluefuton left a comment

Choose a reason for hiding this comment

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

🚢

@davewhitley
Copy link
Contributor

@mladenplavsic feel free to squash and merge.

@mladenplavsic
Copy link
Contributor Author

@drw158 I would like to, but I don't have permission for it :-)

@bluefuton
Copy link
Contributor

I'll do the honours. Thanks @mladenplavsic!

@bluefuton bluefuton merged commit 1b41493 into Automattic:master May 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants