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

Skip adding width/height attributes to images #144

Closed
wrboyce opened this issue Mar 26, 2024 · 8 comments
Closed

Skip adding width/height attributes to images #144

wrboyce opened this issue Mar 26, 2024 · 8 comments

Comments

@wrboyce
Copy link
Contributor

wrboyce commented Mar 26, 2024

I have what I suspect to be something of an edge-case on my hands. We have an SVG which draws a text-based logo (so embedded in a paragraph of text) so it is sized using css and rem units. Unfortunately this causes jampack to make some pretty wild assumptions and breaks the flow of the text.

The best solution I can come up with is either an option to disable adding sizes to svg img tags, or a custom attribute on img tags to indicate to jampack that adding size attributes should be skipped. If one of these sounds appealing I don't mind putting a PR together.

@georges-gomes
Copy link
Member

Do you mind copy pasting an extract of the HTML and SVG? I will provide a quick feedback

@wrboyce
Copy link
Contributor Author

wrboyce commented Mar 26, 2024

HTML:

<p class="fw-bold text-uppercase">partnered with <img class="img-fluid visa-header" src="/img/Visa_Brandmark_White_RGB_2021.svg" alt="VISA Logo"></p>

CSS:

.visa-header {
  height: .875rem;
  margin-bottom: .125em;
  margin-left: .25rem;
}

SVG:

<?xml version="1.0" encoding="utf-8"?>
<!-- Generator: Adobe Illustrator 26.3.1, SVG Export Plug-In . SVG Version: 6.00 Build 0)  -->
<svg version="1.1" id="Layer_1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px"
     viewBox="0 0 1920 620.1" style="enable-background:new 0 0 1920 620.1;" xml:space="preserve">
<style type="text/css">
    .st0{fill:#FFFFFF;}
</style>
<path class="st0" d="M729,11L477.6,610.7h-164L189.9,132.1c-7.5-29.5-14-40.3-36.9-52.7c-37.3-20.2-98.9-39.2-153-51L3.7,11h264
    c33.6,0,63.9,22.4,71.5,61.2l65.3,347L566,11L729,11L729,11z M1371.6,414.9c0.7-158.3-218.9-167-217.4-237.7
    c0.5-21.5,21-44.4,65.8-50.2c22.2-2.9,83.5-5.1,152.9,26.8l27.2-127.2C1362.9,13,1314.9,0,1255.1,0c-153.3,0-261.3,81.5-262.2,198.2
    c-1,86.3,77,134.5,135.8,163.2c60.5,29.4,80.8,48.3,80.5,74.5c-0.4,40.2-48.2,58-92.9,58.7c-78,1.2-123.2-21.1-159.3-37.9
    l-28.1,131.4c36.2,16.6,103.2,31.1,172.5,31.9C1264.5,620.1,1371.1,539.6,1371.6,414.9 M1776.5,610.7H1920L1794.8,11h-132.4
    c-29.8,0-54.9,17.3-66,44l-232.8,555.7h162.9l32.3-89.6h199.1L1776.5,610.7z M1603.4,398.2l81.7-225.2l47,225.2H1603.4z M950.7,11
    L822.4,610.7H667.2L795.6,11L950.7,11L950.7,11z"/>
</svg>

@georges-gomes
Copy link
Member

Wow so I guess jampack add width="1920" and heigth="620" the the <img> tag, is it?

@wrboyce
Copy link
Contributor Author

wrboyce commented Mar 26, 2024

It is adding height="150" and width="300", from here I think.

EDIT: I've just noticed the viewBox stuff in the SVG (I'm not a design/SVG guy by any means!), so I'm not sure why it's not adding the sizes you mentioned!

@georges-gomes
Copy link
Member

It is adding height="150" and width="300", from here I think.

EDIT: I've just noticed the viewBox stuff in the SVG (I'm not a design/SVG guy by any means!), so I'm not sure why it's not adding the sizes you mentioned!

Probably a bug in jampack.

To answer your question:
I like that jampack can work out of the box. And it seems that my implementation for SVG is not good enough for that.
So I would add an option to "enable width and height on SVG". If true => use current code, If 'false => don't add anything.
And I would make this option to false by default.

If you want to push a PR, it's more than welcome.

@georges-gomes
Copy link
Member

Could be named like that in configuration:

{
  ...
   image: {
        svg: {
             ...
             add_width_and_height: boolean;
             ...
        }
 ...
}

Open to alternative proposals.

wrboyce added a commit to wrboyce/jampack that referenced this issue Mar 26, 2024
@wrboyce
Copy link
Contributor Author

wrboyce commented Mar 26, 2024

Thanks @georges-gomes! Can you drop me a ping when you publish a new version including this patch, please?

@wrboyce
Copy link
Contributor Author

wrboyce commented Mar 26, 2024

Ah, just seen your comment on the PR. Thanks!

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

No branches or pull requests

2 participants