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

Suggestion: Further Default Normalization Styles #161

Closed
bhadaway opened this issue May 1, 2022 · 8 comments · Fixed by #172
Closed

Suggestion: Further Default Normalization Styles #161

bhadaway opened this issue May 1, 2022 · 8 comments · Fixed by #172
Labels
enhancement New feature or request

Comments

@bhadaway
Copy link

bhadaway commented May 1, 2022

.si {
	display: inline-block;
	font-family: 'Simple Icons', sans-serif;
	font-style: normal;
	line-height: normal;
	vertical-align: middle;
}

PS: Unless I'm just being a complete dumb-dumb, the actual /font/ folder appears to be missing from the GitHub repo? I had to go to unpkg.com to find the font files and download them.

@mondeja
Copy link
Member

mondeja commented May 3, 2022

Nope, the folder is there. Are you really sure that you're not getting it? This is the output in my console:

$ npm i --no-save simple-icons-font
$ tree node_modules/simple-icons-font
node_modules/simple-icons-font
├── DISCLAIMER.md
├── font
│   ├── simple-icons.css
│   ├── SimpleIcons.eot
│   ├── simple-icons.min.css
│   ├── SimpleIcons.otf
│   ├── SimpleIcons.ttf
│   ├── SimpleIcons.woff
│   └── SimpleIcons.woff2
├── LICENSE.md
├── package.json
└── README.md

1 directory, 11 files

@mondeja mondeja added the awaiting reply Issues or pull requests awaiting reply from an individual before it may be addressed label May 3, 2022
@bhadaway
Copy link
Author

bhadaway commented May 3, 2022

Oh, I see. The fonts are generated dynamically through installation, for obvious practical reasons? They're not just sitting in a folder plainly, within the repo. I'm setting them up manually and just needed to straight download them, so that's why I was a bit confused.

@mondeja mondeja removed the awaiting reply Issues or pull requests awaiting reply from an individual before it may be addressed label May 3, 2022
@mondeja mondeja closed this as completed May 3, 2022
@mondeja mondeja reopened this May 3, 2022
@mondeja
Copy link
Member

mondeja commented May 3, 2022

Could you explain why we must add the following CSS properties?

        font-style: normal;
	line-height: normal;
	vertical-align: middle;

@mondeja mondeja added the awaiting reply Issues or pull requests awaiting reply from an individual before it may be addressed label May 3, 2022
@bhadaway
Copy link
Author

bhadaway commented May 3, 2022

I threw together a quick demonstration:

https://bryanhadaway.com/testing/simple-icons.html

@mondeja
Copy link
Member

mondeja commented May 3, 2022

Thanks, but are the font-style: normal and line-height: normal properties needed? I can see the icon correctly positionated as well if I remove them:

demo-vertical-align

@mondeja mondeja added the enhancement New feature or request label May 3, 2022
@bhadaway
Copy link
Author

bhadaway commented May 3, 2022

Yes, they're needed. This is just a vanilla test, so doesn't take into account some common variables that might take place out in the wild.

font-style: normal protects the icons from warping if they're added between <em></em> tags. Note: this is already in the existing style reset.

line-height: normal protects the icons from overflow problems that can occur (especially on mobile) if the user has set the icons to a big enough size.

I even ran into an issue where I needed to use text-shadow: none, but that probably really is too aggressive in terms of finding the right balance for most users.


For what it's worth, I actually came up with my own hybrid solution where I combined the font method with actual svgs so that I could benefit from a lighter front-end load (the font method adds over 1mb to page load, and my method brings that down to under 100kb) and the responsiveness of svgs. Anyway, that's a discussion for another day, if anyone's interested.

@mondeja
Copy link
Member

mondeja commented May 4, 2022

Thank you for the report, but I think that:

  • If an user puts an icon inside em tags it goes without saying that he wanted it to appear in italics. I suspect that the wrapping problem has to do with the source itself, I have opened Regarding italics normalization #162 to track it.

    italic-icon

  • normal is the default value for the line-height property in CSS, so there are no need for that addition, it would be redundant.

For the problem of high load added by fonts, that's true. It would be great to expose the build process to create a customized build, filtering by icons slugs. I've opened #163 to track it.

So I'm fine with adding vertical-align: middle but I want to study before what other icon fonts like FontAwesome do.

@mondeja mondeja removed the awaiting reply Issues or pull requests awaiting reply from an individual before it may be addressed label May 4, 2022
@bhadaway
Copy link
Author

bhadaway commented May 4, 2022

Just a quick reminder that I'm not suggesting the addition of font-style: normal; it's already in the official code:

https://github.com/simple-icons/simple-icons-font/blob/develop/scripts/templates/base.css

The logic (I would assume) would be that while it's technically a font, that that's only a formality as a trick/method for delivering icons that shouldn't otherwise be treated like text. The same logic applies to line-height: normal and maybe there should even be font-weight: normal too. The icons should only appear exactly as intended concerning the shape/design of each respective icon, without warping/stylization of any kind beyond size and color.

Anyway, that's all I have to offer on the subject; I'll let you take it from here.

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants