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

SVG definitions have width in rem which throws console errors #1935

Closed
michjnich opened this issue Jun 20, 2024 · 5 comments · Fixed by #1942
Closed

SVG definitions have width in rem which throws console errors #1935

michjnich opened this issue Jun 20, 2024 · 5 comments · Fixed by #1942

Comments

@michjnich
Copy link
Contributor

The problematic definitions are in this file: https://github.com/jazzband/django-debug-toolbar/blob/0a622dcab4c78ce0f1d339f0e3147a1023350efa/debug_toolbar/templates/debug_toolbar/includes/theme_selector.html#L5

As I understand it, SVGs cannot define height in em or rem, but require pixels.

The current status results in errors in the console on Firefox stating "Unexpected value 1rem parsing width attribute." - 6 errors in total, one for each SVG definition.

Tested on Edge too and that does not seem to produce the errors, so this looks to be browser dependent. Looking at similar issues on other repos, I suspect Safari would also show them, but I am not using apple so cannot verify this.

Happy to make the change and submit a PR if somebody can say which pixel size they would need.

@tim-schilling
Copy link
Member

I'm not able to reproduce this with Firefox 126.0.1 or 127.0.1 on a mac

@tim-schilling
Copy link
Member

I was able to reproduce on Safari.

A PR would be greatly appreciated. I don't think we should use pixels as that breaks the scalability. I think you could wrap the svg in an element that uses the CSS style of width: 1rem; height: 1rem;, then remove the width and height attributes from the svg element. You'll need to play around with how to get it to appear properly though, but that will handle the sizing issue.

@michjnich
Copy link
Contributor Author

For reference, I was getting this on Firefox on the work laptop, which is currently 115.12.0esr, if that makes any difference. Windows 10 enterprise build 19045.4529, v22H2.

Anyway, I'll see if I can find some time to play with it - not sure if there are any knock on implications of wrapping the svg in a div, so that probably needs some "mucking around time" factored in and am quite busy the next few weeks. I take your point about pixels though.

@michjnich
Copy link
Contributor Author

michjnich commented Jul 4, 2024

OK, I can't get the wrapping with a div to work and display properly for some reason.

I have discovered though that simply changing to use css properties rather than the svg height and width works, because SVG attributes are more strictly defined and teh CSS is a bit broader, so I think this is the best (and least intrusive solution).

So basically style="width: 1rem; height 1rem" - or adding an svg-icon class with these properties and using that.

I'll sort out a PR with a new class later today, unless there are any objections to this change.

Edit.

Actually, I might add it to this section of the css in toolbar.css ...

#djDebug[data-theme="light"] #djToggleThemeButton svg.theme-light,
#djDebug[data-theme="dark"] #djToggleThemeButton svg.theme-dark,
#djDebug[data-theme="auto"] #djToggleThemeButton svg.theme-auto {
    display: block;
    height: 1rem;
    width: 1rem; 
}

@tim-schilling
Copy link
Member

Thanks Michael!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants