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

Convert ellipses circles #702

Closed
wants to merge 3 commits into from
Closed

Convert ellipses circles #702

wants to merge 3 commits into from

Conversation

alexjlockwood
Copy link
Contributor

The ability to convert circles and ellipses to paths has been requested a few times, for example in #652.

This pull request adds the ability to do just that.

LMK what you think!

@strarsis
Copy link
Contributor

strarsis commented Apr 9, 2017

Is this similar to this PR?: #652

@alexjlockwood
Copy link
Contributor Author

@strarsis Yeah, except instead of creating a new plugin it adds the functionality to the existing plugin.

@alexjlockwood
Copy link
Contributor Author

Also, sorry... the tests are failing because of jshint errors (I ported it from a personal project I wrote in es6 and forgot to fix the hoisting stuff :P). Easy to fix... but I'll wait to hear back on whether or not you want me to continue following through on this.

@drewnoakes
Copy link
Contributor

Is there anything blocking this PR other than the jshint errors? I would be happy to address those if it'll get a PR merged. Can add unit tests too.

@GreLI
Copy link
Member

GreLI commented Sep 29, 2017

Well, in general <path>'ll be longer then <circle> and <ellipse>—that's why I didn't it initially. So, there should be an option, I think. Like convertCircles, disabled by default.

@drewnoakes
Copy link
Contributor

Here's a concrete example of the increase in size, assuming integral parameters:

<circle cx="100" cy="100" r="50"/>

<path d="M100 50a50 50 0 1 0 0 100 50 50 0 1 0 0-100z"/>

There might be cases where converting circles to paths allows further path merging to occur, resulting in a net gain. I haven't seen any mention of optimisation heuristics/backtracking though.

So adding a default-disabled option seems harmless, but I'm not sure whether it'd ever make sense to turn it on.

@GreLI
Copy link
Member

GreLI commented Sep 29, 2017

Yes it might, but that is not certain.

@geekbleek
Copy link

I have a number of use cases for converting circles and ellipses - ultimately because I'm not trying to optimize file size - I'm trying to optimize compatibility of all my SVG files. If I can help with this PR please let me know!

@GreLI
Copy link
Member

GreLI commented Oct 27, 2017

Few issues are here:

  • it should be optional
  • there're errors in code, it doesn't work
  • lack of tests.

@strarsis
Copy link
Contributor

@alexjlockwood: There are jshint errors which prevent the tests to pass:
https://travis-ci.org/svg/svgo/jobs/219732094#L246

@alexjlockwood
Copy link
Contributor Author

Sorry about that, will try to fix that up now. :)

@alexjlockwood
Copy link
Contributor Author

Blah... looks like I accidentally deleted my svgo fork. Is it possible to update the pull request if I've done that? Or will I have to create a new one...

@GreLI
Copy link
Member

GreLI commented Oct 27, 2017

No idea, I'm afraid you have to make a new PR.

@alexjlockwood
Copy link
Contributor Author

Going to close this and continue the work here: #818

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.

5 participants