Decathlon / vitamin-web

Decathlon Design System UI components for web applications
https://decathlon.github.io/vitamin-web
Apache License 2.0
282 stars 76 forks source link

question: icons are greater than SVG parent element #1223

Closed ghost closed 2 years ago

ghost commented 2 years ago

Describe the bug

The display of the flags (maybe other icons) is greater than the SVG parent element.

Here is an example with SVG element with height/width sizes to 16px

Screenshot 2022-07-27 at 11 38 21 AM Screenshot 2022-07-27 at 11 54 18 AM

Steps to reproduce

Here is the steps to reproduce (Vanilla React with Webpack)

  1. Add the package @vtmn/assets
  2. Add the Webpack File loader to the config
// For example
{
  test: /\.svg$/,
  use: [
    {
      loader: 'file-loader',
      options: {
        sourceMap: isEnvDevelopment,
      },
    },
  ],
},
  1. Use the package inside the project code (sprite way)
import flags from '@vtmn/assets/dist/assets/sprite/assets.svg';

// ...

return (
  <svg width="16" height="16" fill="#001018">
    // I use href because xlink:href is deprecated
    // cf: https://developer.mozilla.org/en-US/docs/Web/SVG/Element/use
    <use href={`${flags}#flag-at`} />
  </svg>
);
  1. Check inside the browser

Expected behavior

The icon element MUST have the same size as the SVG element have.

Browsers affected

Version affected

@vtmn/assets: v0.1.3

lauthieb commented 2 years ago

Hi @jpogeant, thanks for your issue. I think it is not a bug from Vitamin side, but let's check together. Can you please apply the advice from this answer and let me know if it's better? https://stackoverflow.com/a/39062462 Thanks in advance :)

lauthieb commented 2 years ago

Hi @jpogeant, is it okay for you? @thibault-mahe feel free to check if my answer is correct in your opinion. Thanks!

lauthieb commented 2 years ago

This is a stale issue. I close it, feel free to reopen if needed.

ghost commented 2 years ago

Hi,

I tested using the advice from the StackOverflow answer, without success. The sprite seems to have a mismatching between viewBox et symbol sizes.

Using dynamic imports and SVGs directly this seems to be better.

thibault-mahe commented 2 years ago

Hi @jpogeant , I tried locally with the config you gave (except that I use the Asset Modules webpack config with Nextjs) and it seems to work well:

image

There's been some change recently on the icons assets, with a PR removing the height/width of the svg. Maybe this is linked to your issue. Do you have the last release of the @vtmn/assets ?

ghost commented 2 years ago

Hi @thibault-mahe

Everything is working know 🙌

I think removing height/width was the thing!