Skip to content

Conversation

@mfeckie
Copy link

@mfeckie mfeckie commented Nov 8, 2024

The current implementation makes it challenging to style the SVG after generation.

This PR

  • Switches to applying the sizing view viewBox, which makes subsequent CSS based styling easier
  • Adds a class property to the SVG settings struct, allowing direct application of CSS classes too

It does remove the presence of width and height from the generated SVG, so if accepted this might be considered a breaking change

@philipgiuliani
Copy link

Just wanted to propose the exact same change. Thanks!

@ondrej-tucek
Copy link
Contributor

@mfeckie sorry for the late reply, I completely missed your PR. I'm just wondering why you want to style the SVG after generation. Can you provide a small example of that?

  • I don't see a better benefit of viewbox than width/height. Maybe I'm missing something...
  • Add class to SVG seems to be good idea but there is a big pitfall behind it. Just consider specificity, now we have an options: background_opacity, background_color and qrcode_color which modify SVG somehow. if we add a class at this point, an ambiguity will arise for users. So there would be two options how to change for example the qrcode color and due to the specificity only one approach would work... This might be confusing for users. If you really want to add class you have to consider it and fix your code.
    Btw what is wrong with styling like svg { fill: "..."; ... } in your css file? Or use some wrapper class .wrapper > svg {...}?

@mfeckie
Copy link
Author

mfeckie commented Nov 12, 2024

The primary reason is that the height/width properties set by the library are based on a scaling integer. This means it's not easy to set a very specific size. One of our use cases is to produce a grid of 'login cards' that are used by young students to sign in to an app. So we want to be able to tightly control the height and width. Setting the viewbox allows us to easily scale them after the fact with CSS classes. This is why I suggested viewbox + class (because we need to apply the class to the SVG directly, not a wrapping element).

Begin able to style SVG's by class makes them very flexible, while changing nothing for most people. We use tailwind and so setting the styling directly as you suggest would be outside our normal usage of CSS.

It's fine if you don't find the change helpful, we're using the fork now and are very happy 😄

@vasspilka
Copy link

vasspilka commented Mar 12, 2025

Hey @ondrej-tucek , we would also like to style the resulting SVG with tailwind classes. Would you please consider merging this in?

Otherwise I'll also have to make a fork. Right now we are using a "hack" which involves changing the resulting svg with JS

@ondrej-tucek ondrej-tucek requested a review from s-m-i-t-a March 13, 2025 08:23
@peaceful-james
Copy link
Contributor

  • what is wrong with styling like svg { fill: "..."; ... } in your css file?

You can not expect people to apply SVG styles to all their SVGs, especially since some may come from external sources.

Using a wrapper element is a workaround that ignores the crux of this issue.

It would be much better to let "users" add any HTML attributes they want to the SVG.

@ondrej-tucek ondrej-tucek requested review from s-m-i-t-a and removed request for s-m-i-t-a June 24, 2025 08:02
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 this pull request may close these issues.

7 participants