feat: Remove warning about SVG width and height #1225
Conversation
This warning annoys a lot of people unnecessarily. The argument that this means that the diagram doesn't stretch is true (it won't stretch), but misleading, because stretching is not a good goal to seek. Diagrams in RFCs often have text, which either becomes too large or too small when stretched. Consequently, the right choice is to set width and height in many cases. Indeed, the example in RFC 7992 includes width and height (and no viewbox, but that's a separate issue). RFC 7996 does nothing about this. This warning is therefore overreach.
|
Yes, one of the warnings I regularly silence via: |
|
The RPC has struggled with this warning. Removing height and width attributes requires twiddling with the viewbox attribute to get the image to look sorta right, and this is very much a trial-and-error activity. Even if height and width are set, a reader can still make the image smaller or larger by using the zoom tools in the browser, so it's not like these attributes actually prevent scaling. |
|
That only highlights to me that getting SVG right is tricky (and perhaps in need of clearer guidance). Removing width and height is not going to work in many cases. For instance, I'm not aware of any cases with aasvg where you would want to do that rather than do something else entirely. As Jean notes, zoom tools are probably a better fit from a usability perspective. |
| @@ -926,12 +924,11 @@ def render_artwork(self, h, x): | |||
| if not (svgw and svgh): | |||
| svgw = float(w)-float(xo) | |||
| svgh = float(h)-float(yo) | |||
| elif svgw and svgh: | |||
| svg.set('viewBox', '0 0 %s %s' % (svgw, svgh)) | |||
There was a problem hiding this comment.
I realize this is a pre-existing concern, but is overwriting any viewBox that might have been specified in the source the right thing to do?
Getting a good understanding of reasonable ways to use viewport and viewbox in the series I think needs some discussion.
There was a problem hiding this comment.
The accessibility experts recommend the following for understanding viewport and viewBox: https://www.sarasoueidan.com/blog/svg-coordinate-systems/
There's also https://stackoverflow.com/questions/15335926/how-to-use-the-svg-viewbox-attribute
There was a problem hiding this comment.
My understanding here is that this is fine. What this does is to ensure that there is a viewBox attribute always and that it follows a fixed syntax.
That is, if it is present, then any commas are removed. (That's arm 1.)
If it is not present, but there is a width and height, then one is added based on that width and height. (That's arm 2.)
Otherwise, it's an error (as it likely would be anyway). (That's arm 3).
There is one problem that I see with this code, which is that width and height are not required to be specified in the default unit of pixels. They can be "auto", any length (e.g., "44pt" or "23em"), or a percentage. Use of those will generate ValueError, which will be caught below. That's a fine choice if it were deliberate. However, it means that specifying width="44px" will be treated as invalid, which isn't ideal.
This warning annoys a lot of people unnecessarily.
The argument that this means that the diagram doesn't stretch is true (it won't stretch), but misleading, because stretching is not a good goal to seek. Diagrams in RFCs often have text, which either becomes too large or too small when stretched. Consequently, the right choice is to set width and height in many cases.
Indeed, the example in RFC 7992 includes width and height (and no viewbox, but that's a separate issue). RFC 7996 does nothing about this. This warning is therefore overreach.