-
-
Notifications
You must be signed in to change notification settings - Fork 7
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
fix: apply size to style directly to prevent warnings on size #901
base: dev
Are you sure you want to change the base?
Conversation
b293783
to
7a144d7
Compare
@@ -66,7 +66,7 @@ export default function RssFeed({ serverData, options }: WidgetComponentProps<"r | |||
|
|||
const InfoDisplay = ({ date }: { date: string }) => ( | |||
<Group gap="2.5cqmin"> | |||
<IconClock size="2.5cqmin" /> | |||
<IconClock style={{ height: "3cqmin", width: "3cqmin" }} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide more context why size doesn't work here and why you use the style attribute instead of height and width directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SVG's don't officially support relative sizing, says so in it's spec. Browsers knows that, found it stupid and allows for relative sizing when using the component, but because it's not supposed to support it, it'll flood an error in the console of the browser. so to circumvent that, you need to set the size using the style parameter, which is anoying, especially since the size parameter exist but you just can't use it properly without it making a mess in the browser's logs. (Tabler icons directly apply the size parameter to the height/width parameter of the underlying icon svg, instead of passing it to the styling, so only icons are affected and need to be changed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we contribute this fix upstream then? (eg. Tabler Icons?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah... I agree, just out of laziness I didn't want to bother. But since I don't know how long this will take, and I don't want my console to be flooded, I'd suggest we do this in the meantime.
Here's the code health analysis summary for commits Analysis Summary
|
Coverage Report
File Coverage
|
What is the state of this pr @SeDemal @manuel-rw ? |
I still think that we should contribute upstream. My opinion hasn't changed |
Leave as a keepsake that we need to do it at some point. |
09ac2bf
to
c37a0e3
Compare
Settings tabler icons-react SVG's size parameter using relative units causes warning in console.
Use style instead. (Only affects SVG/icons)