search

svg / svgo

⚙️ Node.js tool for optimizing SVG files
https://svgo.dev/
MIT License
20.93k stars 1.39k forks source link

What is the benefit of stripping viewBox? #1128

Closed avwie closed 4 months ago

avwie commented 5 years ago

Some libraries use SVGO for optimizing the SVG and I notice that stripping the viewBox is almost always used. However, I always disable stripping because it breaks proper scaling of the SVG. Unless I totally don't understand, what is the 'optimization' that stripping a viewBox provides?

Darkproduct commented 1 year ago

I have nothing more to add but this was a much needed break after fiddling the first time with SVGs in a web-context coming from Latex. I tried every setting of width and height in CSS and in the <svg> tag, I read webpage after webpage of tutorials on how to get this thing, which has SCALABLE in its name to scale. Then I found this. Manually added a viewBox Attribute to all my SVG files with 0 0 [width] [height] and now everything is working fine. I used the last 2 hours to relax and to read this issue, because at this point this isn't an issue this is comedy.

Crotalus commented 1 year ago

Wasted a couple of minutes on this problem today wondering why the SVGs were scaled incorrectly.

(on the other hand, it led me into this Battle Thread filled with Don Quixotes🍿)

lzl124631x commented 1 year ago

Ok. I commented in this thread back on Sep 10, 2021. And I have been 🍿 watching we "minority of users" complaining in this thread. It has been fun until this freaking issue kicks my ass again today. I was using SVGR to render SVG as React component and I found that the SVG file loses viewport attribute thus can't resize properly.

voice in my head -> SVGO: "SURPRISE M...F...r!"

And I have to add a stupid svgo.config.js with configs in https://github.com/svg/svgo/issues/1128#issuecomment-978136077 thanks to @GreLI's ignorance and arrogance perseverance.

SVGO

Lucienest commented 1 year ago

Ok. I commented in this thread back on Sep 10, 2021. And I have been 🍿 watching we "minority of users" complaining in this thread. It has been fun until this freaking issue kicks my ass again today. I was using SVGR to render SVG as React component and I found that the SVG file loses viewport attribute thus can't resize properly.

voice in my head -> SVGO: "SURPRISE M...F...r!"

And I have to add a stupid svgo.config.js with configs in #1128 (comment) thanks to @GreLI's ~ignorance and arrogance~ perseverance.

SVGO

Thanks, for ignorance and arrogance

ADTC commented 1 year ago

This looks like a community-maintained repository. Shouldn't this issue be put to vote?

On one hand, disabling removeViewBox seems to be a more sensible default. On the other, doing so would be a breaking change which affects all projects that didn't override it in config, if they would upgrade to a new release with the change.

We could require the plugin to be always specified in config with true or false, forcing all upgradees to decide which behavior they prefer instead of being surprised by a change in the default behavior. (I realize this isn't ideal for CLI usage.)

nnmrts commented 1 year ago

@ADTC It's not "community-maintained" in a sense that we could just "put" this issue to vote.

The most "active" maintainer nowadays is @TrySound, who seemingly abandoned this repo a couple of months ago. Seems like @GreLI also has rights to merge a PR, but we know their position...

We can't find out exactly who has admin access to this repo but there are two more @svg organization members (@rpominov and @deepsweet), both seem to not care about this repo anymore as well, otherwise a crazy person like @GreLI wouldn't have admin access to it anymore.

I'm just waiting for the moment @GreLI snaps and unpublishes the package or publishes it with some russian propaganda (something similar happened with another package in the past), so more people will notice and work on a fork.

So yeah, the only realistic option here is a fork, I'd love to work on one but don't even have time to set it up.

deepsweet commented 1 year ago

I'm the original author as it says in the package.json. I have all the possible access both here and on NPM, but not the time and necessary context to continue working on the project. For many years.

At the same time I can't just blindly merge PRs to the project with 15M NPM downloads per week, so I've tried to look for maintainers over the years. It's way more complicated than it seems to be, starting from the observation that there are no many candidates. In fact, there are none. One thing is to merge old and stalled for months PRs, and another is to develop it all further and push forward.

The only realistic way to dramatically change this situation is to contribute and be ready to become a new maintainer.

Talk to me.

ADTC commented 1 year ago

@deepsweet thank you so much for coming back to chime in! I guess the position of active maintainer is still open and unlikely to be filled any time soon.

In the meanwhile though, do you mind helping us update the Readme? I'd like to start with my own PR #1731 which organizes the content to be more readable without adding anything new.

There's a discussion in the same PR about how to inform users of quirks like this, which I think would be best resolved by adding a FAQ or Troubleshooting section in the Readme. I can add it to the same PR or open a new one.

I think changes to the Readme are much safer to merge than changes to the code. The Readme will also reflect in the NPM repository site so it is worth updating with new and important information.