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

IE7 requires html { font-size: 100%; } in establish-baseline() #1520

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

IE7 requires html { font-size: 100%; } in establish-baseline() #1520

wants to merge 2 commits into from

Conversation

JohnAlbin
Copy link
Contributor

I was checking out the updated vertical-rhythm module and saw that * html { font-size: 100%; } is only added for IE 6. I seemed to remember that IE 7 also required that. So I looked it up…

IE 6 and 7 cannot resize fonts set with px. http://alistapart.com/article/howtosizetextincss#section3

IE 6 and 7 resize fonts weirdly when set with em. http://alistapart.com/article/howtosizetextincss#section4

Since IE7 doesn't understand the * html selector hack, we have to switch to the *property hack that both IE 6 and IE 7 know (but IE 8 and above do not.)

@JohnAlbin
Copy link
Contributor Author

Here's a more recent reference that shows it's both IE 6 and IE 7 that need the percent font.
https://github.com/necolas/normalize.css/blob/v1/normalize.css#L62

@@ -95,16 +95,15 @@ $relative-font-sizing: if($rhythm-unit == px, false, true);

// Establishes a font baseline for the given font-size.
@mixin establish-baseline($font-size: $base-font-size) {
$relative-size: 100% * ($font-size / $browser-default-font-size);
Copy link
Member

Choose a reason for hiding this comment

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

Why was this line removed? $relative-size is still called on line 99, and now throws an error in the tests. This line should stay and be added to the new line 105 as well. No reason to duplicate that math.

@mirisuzanne
Copy link
Member

Thanks @JohnAlbin! Can you fix the broken variable, and get the tests passing? There are instructions for writing and running the tests in CONTRIBUTING.md file.

@JohnAlbin
Copy link
Contributor Author

Ok. The tests are "fixed". Except that they are still being reported as broken because the new tests depend on #1524, so that we can actually test when IE7 support is turned off.

@chriseppstein
Copy link
Member

#1524 was rejected, so can we get this rebased off of master?

@chriseppstein
Copy link
Member

I mean stable.

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

Successfully merging this pull request may close these issues.

3 participants