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

[docs]: "dimensions" option is misleading #550

Closed
polarathene opened this issue Apr 9, 2021 · 4 comments · Fixed by #552
Closed

[docs]: "dimensions" option is misleading #550

polarathene opened this issue Apr 9, 2021 · 4 comments · Fixed by #552

Comments

@polarathene
Copy link
Contributor

polarathene commented Apr 9, 2021

🐛 Bug Report

The docs for the option dimensions are misleading?

Remove width and height from root SVG tag.

Default CLI Override API Override
true --no-dimensions dimensions: <bool>

It appears that the CLI override is required, which in itself implies the default at least for the CLI is the opposite?


Originally I ran into this via the webpack loader package and used the CLI to verify that nothing else was contributing to the confusion of what the docs true default for the given description implied.

To Reproduce

  1. Take the example icon.svg from the docs "Getting Started" page:
icon.svg
<?xml version="1.0" encoding="UTF-8"?>
<svg
  width="48px"
  height="1px"
  viewBox="0 0 48 1"
  version="1.1"
  xmlns="http://www.w3.org/2000/svg"
  xmlns:xlink="http://www.w3.org/1999/xlink"
>
  <!-- Generator: Sketch 46.2 (44496) - http://www.bohemiancoding.com/sketch -->
  <title>Rectangle 5</title>
  <desc>Created with Sketch.</desc>
  <defs></defs>
  <g id="Page-1" stroke="none" stroke-width="1" fill="none" fill-rule="evenodd">
    <g
      id="19-Separator"
      transform="translate(-129.000000, -156.000000)"
      fill="#063855"
    >
      <g id="Controls/Settings" transform="translate(80.000000, 0.000000)">
        <g id="Content" transform="translate(0.000000, 64.000000)">
          <g id="Group" transform="translate(24.000000, 56.000000)">
            <g id="Group-2">
              <rect id="Rectangle-5" x="25" y="36" width="48" height="1"></rect>
            </g>
          </g>
        </g>
      </g>
    </g>
  </g>
</svg>
  1. Run the command on it: npx @svgr/cli icon.svg:
import * as React from "react";

function SvgIcon(props) {
  return (
    <svg width={48} height={1} xmlns="http://www.w3.org/2000/svg" {...props}>
      <path d="M0 0h48v1H0z" fill="#063855" fillRule="evenodd" />
    </svg>
  );
}

export default SvgIcon;
  1. Run the command on it: npx @svgr/cli --no-dimensions icon.svg:
import * as React from "react";

function SvgIcon(props) {
  return (
    <svg viewBox="0 0 48 1" xmlns="http://www.w3.org/2000/svg" {...props}>
      <path d="M0 0h48v1H0z" fill="#063855" fillRule="evenodd" />
    </svg>
  );
}

export default SvgIcon;
  1. Diff the results, note that width/height have been removed with step 3 output:
5c5
<     <svg width={48} height={1} xmlns="http://www.w3.org/2000/svg" {...props}>
---
>     <svg viewBox="0 0 48 1" xmlns="http://www.w3.org/2000/svg" {...props}>
  1. Refer to the dimensions option description in docs describing the functionality and that it defaults to true. Output implies that is not the case.

  2. Verify default is in fact dimensions: true from source:

dimensions: true,

Expected behavior

By default, the root SVG tag should have the width and height attributes removed as implied by the docs description for the dimensions option.

EDIT: Now that I've identified it was originally documented as false (as in by default it does not remove dimensions, despite the internal mapping being dimensions: true and dimensions: false actually meaning to enable the removal feature) but later changed to "avoid confusion" by matching the internal bool without updating the docs description.... The expected behaviour is for the docs to be corrected.

I'll create a PR 👍

Run npx envinfo --system --binaries --npmPackages @svgr/core,@svgr/cli,@svgr/webpack,@svgr/rollup --markdown --clipboard

*** Clipboard option removed - use clipboardy or clipboard-cli directly ***

## System:
 - OS: Linux 5.4 Manjaro Linux
 - CPU: (4) x64 Intel(R) Core(TM) i5-6500 CPU @ 3.20GHz
 - Memory: 17.29 GB / 31.30 GB
 - Container: Yes
 - Shell: 5.1.0 - /bin/bash
## Binaries:
 - Node: 14.4.0 - ~/.asdf/installs/nodejs/14.4.0/bin/node
 - Yarn: 1.22.4 - ~/.asdf/installs/nodejs/14.4.0/.npm/bin/yarn
 - npm: 6.14.5 - ~/.asdf/installs/nodejs/14.4.0/bin/npm
@open-collective-bot
Copy link

Hey @polarathene 👋,
Thank you for opening an issue. We'll get back to you as soon as we can.
Please, consider supporting us on Open Collective. We give a special attention to issues opened by backers.
If you use SVGR at work, you can also ask your company to sponsor us ❤️.

@polarathene

This comment has been minimized.

@polarathene
Copy link
Contributor Author

Investigated this issue further. Some sections repeat stuff from others as I wrote notes ad hoc and tidied up afterwards.

Not enough time to revise this to be terser, but the TL;DR should be sufficient. Perhaps remove the config option with a major release when upgrading to SVGO v2 support. Docs should be updated in the meantime..

Source

Here is the actual logic related to the config option:

if (!opts.dimensions) {
toRemoveAttributes = [...toRemoveAttributes, 'width', 'height']
}

export function getBaseSvgoConfig(config) {
const baseSvgoConfig = {
plugins: [{ prefixIds: true }],
}
if (config.icon || config.dimensions === false) {
baseSvgoConfig.plugins.push({ removeViewBox: false })
}
return baseSvgoConfig
}

[
removeJSXAttribute,
{ elements: ['svg', 'Svg'], attributes: toRemoveAttributes },
],

As well as requiring default dimensions: true for the icon option to replace the width and height attributes with 1em values via this plugin:

...(opts.icon && opts.dimensions ? [svgEmDimensions] : []),


Observations

  • Order of operations = SVGO: removeDimensions => removeViewBox, then SVGR dimensions: false triggering the relevant babel transform.
    • removeDimensions: true: width and height are removed (unless only 1 dimension exists and no viewBox is present). If there is no existing viewBox, and both dimensions are provided, a viewBox will be generated and added: viewBox="0 0 <width> <height>". If a viewBox was already present, if width or height attributes are present they will be dropped, unlike removeViewBox, a mismatch doesn't block removal.
    • removeViewBox: true: viewBox is only removed if it matches both width and height attribute values.
    • dimensions: false: Strips the width and height attributes after processing through SVGO, implicitly disables removeViewBox but does not implicitly enable removeDimensions (avoids potentially generating a viewBox).

Relevant SVGO config variants:

  • A: {removeDimensions: false}, {removeViewBox: false} (Default SVGR config, due to dimensions: false setting removeViewBox: false)
    • Remove nothing.
    • dimensions: false will remove any dimensions after SVGO processing.
  • B: {removeDimensions: false}, {removeViewBox: true} (Default SVGO config. SVGR default dimensions: true matches this)
    • If both dimensions exist and match viewBox values, the viewBox will be removed.
    • dimensions: false will remove any dimensions after SVGO processing.
      • Which can result in no width + height and no viewBox (only if you have explicit config to override SVGR setting removeViewBox: false).
  • C: {removeDimensions: true}, {removeViewBox: false} (What the SVGR docs incorrectly imply dimensions: true does)
    • Dimensions: Keep Never, ViewBox: Keep/Add Always (when possible)
    • Existing viewBox will be used, or one will be generated if both width and height were provided in the source file.
  • D: {removeDimensions: true}, {removeViewBox: true} (same as C)
    • Dimensions are removed first, viewBox cannot be removed as a result.

Problem

As a new user, I wanted the opposite of B - No width or height, keep/generate viewBox if possible:

  • Configuring to C or D for SVGO would work, but SVGR docs implied the default SVGR config already represented either of those.
  • Setting dimensions: false to flip the documented behaviour results in config A, as this setting implicitly disables removeViewBox.
  • If configuring SVGO directly instead for SVGR, one might be aware that:
    • removeDimensions may result in a viewBox being generated.
    • removeViewBox may not remove itself.
    • That both dimensions and viewBox may be retained.
    • But not SVGR dimensions: false having potential to strip both (aka B), or giving the impression of C or D when it's actually A (with no explicit SVGO config, just relying on known SVGO defaults and what SVGR describes dimensions as doing).

History

The dimensions option feature was contributed in March 2018, it has the default value as true in src/configToOptions.js, and documented the default as false, which someone noticed and changed to true in Jan 2019. The SVGO plugin removeDimensions later added the viewBox generator logic in July 2019.

The Jan 2019 docs change seems to be the source of the issue. They cited the mismatch as potential for confusion, which while valid, ignored updating the docs description. This also explains the mixed message the CLI override implies.

The correct fix would have been to assign a more clear intent to the variable name (keepDimensions or removeDimensions), but as it wasn't caught when introduced/released, that would now be a breaking change. For now, correcting the docs is the best approach, or keeping dimensions around but introducing a replacement that it maps to (maintenance and docs burden).


Scaling behaviour of SVG in <svg> vs <img> with dimensions and/or viewBox

  • Unlike this PR comment stated, this is not 100% width/height, it varies across browsers, but the most common is svg {width: 300px; height: 150px;}. (EDIT: They were stating you can specify width/height without a viewBox present, but only when a viewBox is present - is it equivalent to {width: 100%; height: 100%;} which is kind of true, but the viewBox maintains aspect ratio regardless of what it's elements width/height aspect ratio is)
  • If you retained a viewBox, this allows scaling to a given width + height, or maintaining the aspect ratio if you only provide one dimension.
    • With a viewBox there's also preserveAspectRatio which enables an equivalent to CSS object-fit + object-position props for inline SVG, should the default scaling behaviour not be desired.
      • <img> can still use preserveAspectRatio (which applies via the browser default object-fit: fill), but can additionally override it with those two CSS equivalent props as you would any other image content.
      • preserveAspectRatio works with width and height being defined on the <img> element attribtues and/or in combination with it's CSS width + height. Unlike inline <svg>, this allows for leaving the SVG with no width and height to define an aspect ratio differing from viewBox for preserveAspectRatio to work.
        • Note if the SVG only has a single dimension defined, then the aspect ratio has not changed, it's just adjusted the default scale by that dimension, adding the other dimension via attribute or CSS to <img> will not trigger preserveAspectRatio, it'll just scale the content again, both width and height must be set in either the <img> or internal SVG boundary to introduce the contrasting aspect ratio to adjust the viewBox behaviour to.
  • Without a viewBox you are defining a "viewport" area for the element, which by default is aligned top-left (0,0 x/y co-ordinates) within the SVG document/element, adding excess padding/white-space or clipping the region (provided a browsers user-agent applies the default overflow:hidden to svg, since the internal SVG content doesn't scale without a viewBox (It doesn't AFAIK with inline <svg> or an <img> with data URI).
  • With an <img> referencing the SVG via the src attribute, be that external file or data URI, if either width, height or both attributes are defined in the SVG, even if there is no viewBox you can get scaling by adjusting the equivalent SVG width and height attributes on the image element.
    • For a single dimension only it will stretch, otherwise if both were defined and you deviate from the aspect ratio you can use object-fit and object-position to adjust scaling behaviour from the default fill (stretch/distort to fit).
    • If the <img> internal SVG did not define the width or height, then changing that undefined dimension (via img attribute or CSS) just results in "viewport" area adjustment behaviour described earlier preventing any scaling on that dimension.

dimensions: false no longer relevant?

Effectively, SVGR offers an option to always strip width and height attributes from the root SVG element after SVGO processing (that is depending on content, a user may not get consistent output expected).

Unlike the removeDimensions plugin, it will not cause a viewBox to be generated if the SVG is missing one, and will always remove dimensions even if no viewBox exists and only 1 dimension is provided. Note if no viewBox exists, but both width and height attributes do, removeDimensions: true with removeViewBox: false will also give you that "clean slate".

I think it is unlikely to encounter an SVG with a single dimension and no viewBox defined. If one is defined, dimensions: false does not help remove the viewBox anyway (it explicitly disables viewBox removal, but even with removeViewBox: true, removal will not occur when only a single width or height is defined).

When is this actually relevant in practice? If you want no dimensions or viewBox defined at all, and to rely on implicit browser handling or setting your own width and/or height? Unclear when this really applies outside of niche cases.


TL;DR

If SVGO is in use, it should delegate this specific removal to removeDimensions plugin instead? (which if already active can be confusing when mixed with dimensions: false due to processing order)

The option was added mostly to enforce removal of dimensions back when removeDimensions ignored removal if a viewBox was not present. That SVGO plugin now only ignores removal if only a single dimension is provided. A viewBox either already exists or is added if both dimensions are removed.

There isn't much value or reason for the current dimensions config option AFAIK?

  • Without a viewBox it won't scale (unless the SVG is used in say the <img> element and has at least one dimension, but both are required for proper scaling expectations if no viewBox exists).
  • dimensions: false removes both regardless, so is only relevant to inline SVG usage.
  • It disables removeViewBox leaving a viewBox if one does exist. Which can lead to inconsistent expectations of a user vs removeDimensions: true.
  • The dimensions can be set via the generated component to override any existing ones.

@dmatora
Copy link

dmatora commented Sep 23, 2021

So is there a way to keep both dimensions and viewbox?
I've tried all combinations I could think of but one or the other is always removed

To get it working I had to change

  if (config.icon || config.dimensions === false) {

to

  if (!config.removeViewBox || config.icon || config.dimensions === false) {

at node_modules/@svgr/plugin-svgo/lib/config.js:24

and use

        use: [
          {
            loader: '@svgr/webpack',
            options: {
              removeViewBox: false,
            },
          },
        ],

as a config

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 a pull request may close this issue.

2 participants