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

Improve inline SVG detection #1553

Closed
wants to merge 1 commit into from
Closed

Improve inline SVG detection #1553

wants to merge 1 commit into from

Conversation

eltonmesquita
Copy link
Contributor

Fix issue #1552

@patrickkettner
Copy link
Member

@eltonmesquita - I thought you said you did not want to run both checks?

@eltonmesquita
Copy link
Contributor Author

Sorry, I've got a bit confused by your answer. But still, I think it is a better option, as it will not return any false positive, like the current test. If it is to be documented, where exactly this info should be? I don't see anything related to this kind of issue in the docs.

@patrickkettner
Copy link
Member

we usually add a knownBugs field to the detect if there are known problems.

@@ -7,8 +7,12 @@
"notes": [{
"name": "Test page",
"href": "http://paulirish.com/demo/inline-svg"
}, {
"name: "Test page and results",
Copy link
Member

Choose a reason for hiding this comment

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

you have a typo here that is breaking the parsing and thus the tests

"name: should be "name":

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for that, ended up missing it. Fixed!

@patrickkettner
Copy link
Member

Could you squish down everything to a single commit? then we can get this landed :D

@eltonmesquita
Copy link
Contributor Author

Did I do it right, now? I don't know why the AppVeyour build failed, but it doesn't seem to be a problem with the code itself...

@patrickkettner
Copy link
Member

appveyor is shakey - it messed up during the npm install. I kicked off the build again, just to double check.

@patrickkettner
Copy link
Member

you would want to run git rebase -i HEAD~7 and change all but the first pick to fs, then save and force push up the changes. That way the git history on Modernizr stays nice and clean

@eltonmesquita
Copy link
Contributor Author

Did I get it now? Got to take a course on git...

@patrickkettner
Copy link
Member

@eltonmesquita seems like it got messed up :[

sorry again for complicated things. I cherrypicked and merged in via 71e9e73

thanks a ton for the PR!

@eltonmesquita
Copy link
Contributor Author

No problem, this mess happened because I lacked some Git skills and time to review everything, it wasn't your fault. Next time I contribute, I'll do it in a clean way!
Thanks a lot for helping and discussing!

@patrickkettner
Copy link
Member

👍

On Fri, May 1, 2015, 2:02 PM Elton Mesquita notifications@github.com
wrote:

No problem, this mess happened because I lacked some Git skills and time
to review everything, it wasn't your fault. Next time I contribute, I'll do
it in a clean way!
Thanks a lot for helping and discussing!


Reply to this email directly or view it on GitHub
#1553 (comment).

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.

None yet

2 participants