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

Proposal - Fixed width for glypicons #10022

Merged
merged 2 commits into from
Aug 24, 2013

Conversation

thomaswelton
Copy link
Contributor

The problem…
Before the Glyphicon font file has loaded elements using glypicons have no width defined. Then once the font loads the elements grow and move other items around the page.
This issue is exacerbated when after the elements grow they cause text to go onto new lines.

The proposed fix - Add a defined width of 1em to all :empty glyphicons

Demo of the problem http://9a6c381b6ab68b16ed63-5b5693321d576aeab3f1767c0caf2c60.r44.cf3.rackcdn.com/
Demo of the proposed fix http://9a6c381b6ab68b16ed63-5b5693321d576aeab3f1767c0caf2c60.r44.cf3.rackcdn.com/fix.html

thomaswelton and others added 2 commits August 22, 2013 02:54
Prevents elements moving around the page after the font file has loaded by setting a defined width on elements using the icon font.
@thomaswelton
Copy link
Contributor Author

The fact that this is hosted on a CDN doesn't show the problem very well as the fonts are served fast and are cache. But clear your cache before each request and you'll see what first time users will see.
Also would fix issues where the website isn't sending good cache expires headers.

mdo added a commit that referenced this pull request Aug 24, 2013
@mdo mdo merged commit c7db8ea into twbs:master Aug 24, 2013
@mdo
Copy link
Member

mdo commented Aug 24, 2013

Neato, gang. I like it. Thanks!

@mdo mdo mentioned this pull request Aug 24, 2013
@thomaswelton
Copy link
Contributor Author

Cheers, thanks for 3.0 🍻

@ggam
Copy link
Contributor

ggam commented Aug 24, 2013

Shouldn't we use px instead of em (to ensure IE8 support)?

@thomaswelton
Copy link
Contributor Author

@ggam no, because then the width other the glyphicon will only be 1px wide. (try it out in chrome dev tools)
By using ems the width of the icon will equaly the font size.

@juthilo
Copy link
Collaborator

juthilo commented Aug 24, 2013

@ggam IE8 doesn't pick this up anyway, because it doesn't support the :empty selector.

@ggam
Copy link
Contributor

ggam commented Aug 24, 2013

@thomaswelton right, thank you.
@juthilo dind't remember that either.

@cvrebert cvrebert mentioned this pull request Aug 31, 2013
@Tomas-M
Copy link

Tomas-M commented Aug 31, 2013

Please document this at bootstrap website, add examples of empty icon. Thank you

@thomaswelton thomaswelton deleted the proposal-glyphicon-block branch August 31, 2013 10:19
@thomaswelton
Copy link
Contributor Author

For the use case suggested here we would also need to add a height declaration

glyphicon:empty {
    width: 1em;
    height: 1em;
}

Also, we can't really add an empty icon example to the existing list of icons as it messes up the pretty grid

screen shot 2013-08-31 at 11 37 55

Shall something be added to the "How to use" section instead?

@thomaswelton thomaswelton restored the proposal-glyphicon-block branch August 31, 2013 10:41
@thomaswelton
Copy link
Contributor Author

PR for the height declaration #10329
If anyone wants to give a hint as to how this should be discussed in the docs let me know and I'll add that in too.

stempler pushed a commit to stempler/bootstrap that referenced this pull request Apr 11, 2014
…icon-block

Proposal - Fixed width for glypicons
stempler pushed a commit to stempler/bootstrap that referenced this pull request Nov 4, 2014
…icon-block

Proposal - Fixed width for glypicons
@cvrebert
Copy link
Collaborator

cvrebert commented Jul 5, 2015

This was eventually reverted in 2979e4b:

  • Removes the width: 1em on .glyphicon because it makes it
    impractical to resize the width of all icons, even if they’re not
    “empty” (e.g., a simple .glyphicon { width: 30px; } wouldn’t work,
    nor would a class preceeded by a parent class.

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

Successfully merging this pull request may close these issues.

6 participants