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: #31 Fix the defs bug #32

Merged
merged 4 commits into from
Mar 5, 2015
Merged

fix: #31 Fix the defs bug #32

merged 4 commits into from
Mar 5, 2015

Conversation

pastjean
Copy link
Contributor

@pastjean pastjean commented Mar 3, 2015

README.md : update doc to say display none doesnt work in some cases
gulpfile.js : remove display:none injector
inline-svg.html : wrap injected file into hidden div
inline-svg.html : add gradient circle to visually test
index.js : added a global defs , remove inner symbol def

No tests :( sorry

README.md : update doc to say display none doesnt work in some cases
gulpfile.js : remove display:none injector
inline-svg.html : wrap injected file into hidden div
inline-svg.html : add gradient circle to visually test
index.js : added a global defs , remove inner symbol def
@w0rm
Copy link
Owner

w0rm commented Mar 3, 2015

@pastjean thanks, we definitely have to solve this issue, I can't merge this straight away because we have to add tests and also fix my concern regarding the same ids coming from the same files.

As a disclaimer, I didn't consider this to be used for complex svgs, but once we're getting into nested references, we have to make ids of svg contents unique.

@pastjean
Copy link
Contributor Author

pastjean commented Mar 4, 2015

I'll start fixing the simple things right away then the multiple ids shouldnt be to much either!

I will try fixing tests too!

@pastjean
Copy link
Contributor Author

pastjean commented Mar 4, 2015

@w0rm Everything is fixed! Even added a test for <defs/>tag on parent.

let's open a new issue for the multiple IDs thing.

@@ -119,6 +119,7 @@ describe('gulp-svgstore unit test', function () {
var result = file.contents.toString()
var target =
'<svg xmlns="http://www.w3.org/2000/svg">' +
'<defs/>' +
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if we should not output empty in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably , well the grunt one doesnt do , i guessed doing the same was good enough !

@w0rm w0rm merged commit ea95c33 into w0rm:master Mar 5, 2015
jraoult pushed a commit to jraoult/mixtube that referenced this pull request Feb 5, 2017
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.

2 participants