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

Improve font icon management, sizing and sharpness #493

Merged
merged 4 commits into from Jul 22, 2016

Conversation

astorije
Copy link
Member

@astorije astorije commented Jul 12, 2016

I was hoping to make a dead-simple PR to fix the icon blurriness (well, at least on a Mac OS with Retina, not sure how it looks otherwise), but I ended up spending waaaay too much time on this and also improving sizing, management, and some transitions.

Changes

From the commit messages:

  1. Improve font icon rendering with font-smoothing
    This makes all icons look sharper, and reproduces what is applied on the FontAwesome CSS
  2. Define icon font only once and fix sizing
    • This commit applies the following across the whole style:
      • font statement is now the same as official FontAwesome CSS
      • Ensure icons are never italic or bold or that other variants can be applied
      • Ensure font-size and line-height of icons are inherited from parent
      • font-family and font-smoothing is now defined only once
    • A few (mostly positive) side effects from these and related changes:
      • Header icons (main menu, context menu and user list) are now vertically centered!
      • Same applies to the Send icon, but it's more subtle there
      • Alignment of the footer icons are shifted a tiny bit
      • Server window icons are a wee bit bit bigger to match the server name font-size
      • The "Play sound" icon and text are now both 14px (was 14px / 16px)
  3. Add/fix/remove some CSS transitions
    • Transition on the search icon was removed, because why was it even here?!
    • A transition was added to the "Play sound" button
    • Transition on the Send button is now consistent with the others
  4. Centralize all icon definitions for better management

Results

Note that these are are screenshots from Chrome 51 on Mac OS X El Cap with Retina, but I noticed extremely similar (if not identical) results with Firefox on the same machine.
Let me know if you see different results on other OS.

Le What Before After
Sharpness screen shot 2016-07-12 at 02 13 53 screen shot 2016-07-12 at 02 14 02
Sharpness screen shot 2016-07-12 at 02 13 58 screen shot 2016-07-12 at 02 14 09
Sharpness screen shot 2016-07-12 at 02 14 20 screen shot 2016-07-12 at 02 14 25
Text/Icon alignment screen shot 2016-07-12 at 02 15 40 screen shot 2016-07-12 at 02 15 49
Sizing screen shot 2016-07-12 at 02 14 36 screen shot 2016-07-12 at 02 14 49

@astorije astorije added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Jul 12, 2016
@williamboman
Copy link
Member

Nice, LGTM 👍 .

@maxpoulin64
Copy link
Member

Looks all good to me, just look at my comment and then 👍

.context-menu-item:before {
font: normal normal normal 14px/1 FontAwesome;
font-size: inherit; /* Can't have font-size inherit on line above, so need to override */
-webkit-font-smoothing: antialiased;
Copy link
Member

Choose a reason for hiding this comment

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

If we could put subpixel-antialiased there instead it would be pretty great. On my computer it makes a small but noticable difference by making the icon slighly more sharp.

capture d ecran_2016-07-16_14-31-52

Copy link
Member Author

@astorije astorije Jul 17, 2016

Choose a reason for hiding this comment

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

Actually, we cannot: subpixel-antialiased is what is used by default by browsers. So setting it here on icons would kill the point of this PR and go back to ugly rendering :D

Also, note that this is what's used by the Font Awesome CSS, so I'm happy to trust them on this one. As a matter of fact, it's been attempted at different occasions by them or their contributors, see:

Also, this is what Bootstrap does in their CSS for their icons.

This makes all icons look sharper, and reproduces what is applied on the FontAwesome CSS:
https://github.com/FortAwesome/Font-Awesome/blob/4213679/css/font-awesome.css#L19-L20
This commit applies the following across the whole style:

- `font` statement is now the same as official FontAwesome CSS
- Ensure icons are never italic or bold or that other variants can be
  applied
- Ensure font-size and line-height of icons are inherited from parent
- font-family and font-smoothing is now defined only once

A few (mostly positive) side effects from these and related changes:

- Header icons (main menu, context menu and user list) are now
  vertically centered!
- Same applies to the Send icon, but it's more subtle there
- Alignment of the footer icons are shifted a tiny bit
- Server window icons are a wee bit bit bigger to match the server name
  font-size
- The "Play sound" icon and text are now both 14px (was 14px / 16px)
- Transition on the search icon was removed, because why was it even
  here?!
- A transition was added to the "Play sound" button
- Transition on the Send button is now consistent with the others
@astorije
Copy link
Member Author

@maxpoulin64, ping on my answer to your comment? Are you good to go with this PR?

@maxpoulin64 maxpoulin64 merged commit 828289a into master Jul 22, 2016
@maxpoulin64 maxpoulin64 deleted the astorije/improve-fonts branch July 22, 2016 05:22
@astorije astorije modified the milestone: 2.0.0 Aug 6, 2016
matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017
Improve font icon management, sizing and sharpness
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants