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

Icons Package: fix line icons styling #40315

Merged
merged 1 commit into from
Jul 15, 2022

Conversation

arcangelini
Copy link
Contributor

What?

All three line icons (lineDashed, lineDotted, lineSolid) are missing the viewbox and have hardcoded height/width and fill. This seems to be counter to what the others are set as and causes issues when passing a size to the component. I noticed this when using one of them in a project here. All the other icons are changing size except the lines.

Why?

Mostly all other icons listed in this package refrain from setting height/width and fill.

How?

Simply remove the height/width, add the viewbox, and removed the fill. Removing the height/weight is not completely necessary but I chose to do so to match the rest of the icons. The important piece is the viewbox.

Testing Instructions

I personally tested this in my project, but you can also see it in the Storybook

Screenshots or screencast

Before After
Markup 2022-04-13 at 13 57 11 Markup 2022-04-13 at 13 51 40

@skorasaurus skorasaurus added the [Package] Icons /packages/icons label Apr 25, 2022
@skorasaurus skorasaurus requested a review from jasmussen April 25, 2022 19:04
@jasmussen
Copy link
Contributor

I noticed this as well! And thank you for the PR. I'm going to CC some folks that worked on this and see if they can provide some more context. One question to pose also is: should these be part of the icon library? If they are useful enough it makes sense to fix them up like this, but if they are only ever meant to be used as part of the border controls, they could just live alongside that component directly.

Copy link
Contributor

@jameskoster jameskoster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be an oversight, thanks for the fix!

Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this @arcangelini 👍

These icons were introduced via #31585 (6500f68) and were just copied verbatim from the supplied SVG code. I don't believe there was any specific reasoning behind omitting the viewbox attribute etc and this was just an oversight as James noted.

I've tested these changes with the current uses of the icons within the block and site editors. Everything appears ok to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Icons /packages/icons
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants