HatScripts / circle-flags

A collection of 400+ minimal circular SVG country, state and language flags
https://hatscripts.github.io/circle-flags
MIT License
928 stars 248 forks source link

Proposal: use width/height instead of viewBox #7

Closed waldyrious closed 3 years ago

waldyrious commented 4 years ago

I realize that viewBox="0 0 512 512" is shorter than width="512" height="512", but that's only a 3-character difference, and the latter has several advantages:

I'd argue that the pros of width/height outweigh the cons, so I'd like to make the change to svgo.yml and to the svg files in the repo, but I wanted to make sure we're on the same page regarding this beforehand, hence this issue. Let me know what you think!

HatScripts commented 4 years ago

Before discussing your other points, would you mind clarifying what you mean by this? The rich diff you linked to looks valid on my screen: 2020-06-14 02 46 36 Fix_proportions_of_the_CV_flag_by_waldyrious_·_Pul

waldyrious commented 4 years ago

Wait, what?? That's nuts, I never noticed that — but it looks like it may be a browser issue.

Here's what I see on Firefox (v77):

Screenshot 2020-06-13 at 21 11 26

...and Google Chrome (v83):

Screenshot 2020-06-13 at 21 12 04

So yeah, nevermind that argument. I'll try to investigate further and maybe report it to github or the firefox bug traker.

HatScripts commented 4 years ago

Oh, that's really weird. I'm using Opera and it looks fine in the rich diff you linked earlier, but in the regular view of an SVG file it's broken:

2020-06-15T03:01:32+10 circle-flagscv.svg_at_master_·_HatScriptscircle- 2020-06-15T03:05:28+10 circle-flagscv.svg_at_master_·_HatScriptscircle-

HatScripts commented 4 years ago

But it definitely used to work. I think it's a GitHub issue, because it can be fixed by changing the page CSS.

waldyrious commented 4 years ago

Thanks for the additional tests. I'll reach out to GitHub support to report this.

HatScripts commented 3 years ago

As a follow-up to my comment on #11:

I agree with all your points. My only follow-up questions are:

HatScripts commented 3 years ago

Thanks for the additional tests. I'll reach out to GitHub support to report this.

By the way, did you end up finding out what the issue was here?

waldyrious commented 3 years ago

Are you aware of any downsides to using width and height over viewBox?

I can't think of any. Actually, despite my proposal in this issue, I tend to prefer SVGs with relative units (percentages and ratios) rather than absolute ones (pixels, milimeters...), but to be fair that's only really useful for dynamic documents. For static images, using relative units does makes them easier to understand and to edit. In terms of usage, there's no difference — an SVG image usually behaves just like a regular non-vector image file, and can be resized in its container (e.g. an <img> tag) to whatever size the user wishes, so it doesn't matter much if we define its dimensions directly with width and height, or scale the coordinates inside using viewBox, since we usually don't control it's final size anyway. Besides, we are already defining the geometry itself in absolute terms, so it's only fair to make the SVG dimensions be specified in the same way as the actual drawing within.

This page seems to have some relevant information — may be useful to help with experiments in this regard. Additionally, maybe @jjenkov could offer some insight? He has very thorough and informative SVG tutorials, so he may be aware of details we're overlooking.

By the way, did you end up finding out what the issue was here?

Not really; I haven't had the chance to properly prepare a bug request to send to GitHub, and if I'm being honest, I don't feel very motivated to report bugs to closed systems that work sort of like a black hole — you must submit it to their support email, so it doesn't get visible for other people: you can't know if anyone else has reported the same issue, or whether others support your request; and you don't get any way to check the progress or prioritization of the issue. I'd rather "report" it to isaacs/github, as was done with other svg-related issues before.

HatScripts commented 3 years ago

By the way, did you end up finding out what the issue was here?

Not really; I haven't had the chance to properly prepare a bug request to send to GitHub, and if I'm being honest, I don't feel very motivated to report bugs to closed systems that work sort of like a black hole — you must submit it to their support email, so it doesn't get visible for other people: you can't know if anyone else has reported the same issue, or whether others support your request; and you don't get any way to check the progress or prioritization of the issue. I'd rather "report" it to isaacs/github, as was done with other svg-related issues before.

That's completely fair; that reporting system seems like a nightmare. Nevertheless, thank you very much for pointing me in to the right direction and for giving me these additional resources to look into.

  • it allows GitHub to render rich diffs properly (currently GitHub assumes a zero-width, zero-height svg document e.g. in ...)

Something else that seems relevant to this:

SVGs using viewBox don't seem to render at all in Google Sheets, but those using width/height do. That is:

So if GitHub and Google Sheets both have trouble displaying viewBox SVGs, then I would assume there are other sites out there with the same issue.

climech commented 3 years ago

One downside to omitting viewBox entirely is that resizing the icons (either via CSS or HTML attributes) stops working as expected for SVGs embedded directly in HTML (<svg> tag rather than <img>), e.g.:

<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="UTF-8">
  <meta name="viewport" content="width=device-width, initial-scale=1.0">
  <title>Document</title>
  <style>
    svg {
      width: 256px;
      height: 256px;
    }
  </style>
</head>
<body>
  <svg xmlns="http://www.w3.org/2000/svg" width="512" height="512">
    <mask id="a">
      <circle cx="256" cy="256" r="256" fill="#fff"/>
    </mask>
    <g mask="url(#a)">
      <path fill="#0052b4" d="M0 0h144.7l36 254.6-36 257.4H0z"/>
      <path fill="#d80027" d="M367.3 0H512v512H367.3l-29.7-257.3z"/>
      <path fill="#ffda44" d="M144.7 0h222.6v512H144.7z"/>
      <path fill="#d80027" d="M256 354.5V256h66.8v47.3zm-66.8-165.3H256V256h-66.8z"/>
      <path fill="#ff9811" d="M289.4 167a22.3 22.3 0 0 0-33.4-19.3 22.1 22.1 0 0 0-11.1-3c-12.3 0-22.3 10-22.3 22.3H167v111.3c0 41.4 32.9 65.4 58.7 77.8a22.1 22.1 0 0 0-3 11.2 22.3 22.3 0 0 0 33.3 19.3 22.1 22.1 0 0 0 11.1 3 22.3 22.3 0 0 0 19.2-33.5c25.8-12.4 58.7-36.4 58.7-77.8V167zm22.3 111.3c0 5.8 0 23.4-27.5 40.9a136.5 136.5 0 0 1-28.2 13.3c-7-2.4-17.8-6.7-28.2-13.3-27.5-17.5-27.5-35.1-27.5-41v-77.9h111.4z"/>
    </g>
  </svg>
</body>
</html>

produces:

2021-02-09T133200 700957247+0100

Putting the viewBox back fixes this.

Seeing @HatScripts's comment about the Google Sheets issue, I believe the safest approach is to include both viewBox and width/height. This may seem redundant at first glance, but they're quite distinct properties! viewBox defines the document's coordinate system and make it scalable, while width and height set the initial scale.

More on this here: https://css-tricks.com/scale-svg/

waldyrious commented 3 years ago

@HatScripts the GitHub issue actually seems to be resolved; I can't reproduce it anymore. I took the liberty to collapse the comments above that were solely about that issue, to clean up the thread.

@climech that's a good point in favor of viewPort. In your example, the width and height properties defined in CSS override the width and height attributes of the SVG element. I have no objection to using both width/height and viewPort, especially if that results in broader support for various platforms.

HatScripts commented 3 years ago

I've implemented this suggestion with https://github.com/HatScripts/circle-flags/commit/11239c09d6b6fc81c18dcc543f73dbdf17c514b5.

All flags should now have both a viewBox as well as a width and height:

Before After
viewBox="0 0 512 512" viewBox="0 0 512 512" width="512" height="512"